Add support set selection_method for group modification (contiv#66)

Add support to set Propertiese in a GroupMod message. The "Properties" field is set in the message only when the GroupMod message type is add or modify, and it must be empty in other types. The selection_method configurations are maintained as a Property in GroupMod message. Signed-off-by: wenyingd <[email protected]>
wenyingd · Jul 10, 2023 · bea7be7 · bea7be7
1 parent 1905af8
commit bea7be7
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 12 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-v0.8.1
+v0.9.0
diff --git a/go.mod b/go.mod
@@ -3,7 +3,7 @@ module antrea.io/ofnet
 go 1.19
 
 require (
-	antrea.io/libOpenflow v0.11.0
+	antrea.io/libOpenflow v0.12.0
 	github.com/Microsoft/go-winio v0.6.1
 	github.com/contiv/libovsdb v0.0.0-20170227191248-d0061a53e358
 	github.com/orcaman/concurrent-map/v2 v2.0.1

diff --git a/go.sum b/go.sum
@@ -1,5 +1,5 @@
-antrea.io/libOpenflow v0.11.0 h1:CugX6u46zI7qlWf/9HXDSjPpkRgcK46eVko+mLGuxEc=
-antrea.io/libOpenflow v0.11.0/go.mod h1:WNcqu1927fBdc4G9wocmi58+bfxaLmZOTaTGROEcM8I=
+antrea.io/libOpenflow v0.12.0 h1:uLuLERzUN9xS9Ge9I38ixQW344ESx5B6XsG48ZLcNB8=
+antrea.io/libOpenflow v0.12.0/go.mod h1:WNcqu1927fBdc4G9wocmi58+bfxaLmZOTaTGROEcM8I=
 github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow=
 github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM=
 github.com/cenkalti/hub v1.0.1-0.20140529221144-7be60e186e66 h1:mqwgWF7yBJ/zOFlWZk84IRFG/FhMG0f7aZWvcTx/JHA=

diff --git a/ofctrl/fgraphGroup.go b/ofctrl/fgraphGroup.go
@@ -36,6 +36,7 @@ type Group struct {
 	ID          uint32
 	GroupType   GroupType
 	Buckets     []*openflow15.Bucket
+	Properties  []util.Message
 	isInstalled bool
 }
 
@@ -77,6 +78,13 @@ func (self *Group) ResetBuckets(buckets ...*openflow15.Bucket) {
 	}
 }
 
+func (self *Group) AddProperty(prop util.Message) {
+	self.Properties = append(self.Properties, prop)
+	if self.isInstalled {
+		self.Install()
+	}
+}
+
 func (self *Group) Install() error {
 	command := openflow15.OFPGC_ADD
 	if self.isInstalled {
@@ -97,6 +105,7 @@ func (self *Group) Install() error {
 func (self *Group) getGroupModMessage(command int) *openflow15.GroupMod {
 	groupMod := openflow15.NewGroupMod()
 	groupMod.GroupId = self.ID
+	groupMod.Command = uint16(command)
 
 	switch self.GroupType {
 	case GroupAll:
@@ -109,6 +118,16 @@ func (self *Group) getGroupModMessage(command int) *openflow15.GroupMod {
 		groupMod.Type = openflow15.GT_FF
 	}
 
+	if command == openflow15.OFPGC_DELETE {
+		return groupMod
+	}
+
+	if command == openflow15.OFPGC_ADD || command == openflow15.OFPGC_MODIFY {
+		for _, prop := range self.Properties {
+			groupMod.Properties = append(groupMod.Properties, prop)
+		}
+	}
+
 	for _, bkt := range self.Buckets {
 		// Add the bucket to group
 		groupMod.AddBucket(*bkt)
@@ -117,7 +136,7 @@ func (self *Group) getGroupModMessage(command int) *openflow15.GroupMod {
 	if command == openflow15.OFPGC_INSERT_BUCKET {
 		groupMod.CommandBucketId = openflow15.OFPG_BUCKET_LAST
 	}
-	groupMod.Command = uint16(command)
+
 	return groupMod
 }
 

diff --git a/ofctrl/ofctrl_test.go b/ofctrl/ofctrl_test.go
@@ -950,13 +950,82 @@ func TestBundle2(t *testing.T) {
 	assert.False(t, found)
 
 	time.Sleep(2 * time.Second)
-	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", true)
+	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", "", true)
 	matchStr := "priority=100,ip,in_port=11"
 	actionStr := "group:2"
 	assert.Truef(t, ofctlDumpFlowMatch(brName, int(ofActor.inputTable.TableId), matchStr, actionStr),
 		"br: %s, target flow not found on OVS, match: %s, actions: %s", brName, matchStr, actionStr)
 }
 
+func TestGroupWithSelectionMethod(t *testing.T) {
+	brName := ovsDriver.OvsBridgeName
+	tcpSrcPortFieldWithValue := openflow15.NewTcpSrcField(0x3f)
+	tcpSrcPortFieldWithoutValue := openflow15.NewTcpSrcField(0xffff)
+	nwProtoFieldWithoutValue := openflow15.NewIpProtoField(0xff)
+	for _, tc := range []struct {
+		name           string
+		groupId        uint32
+		mt             openflow15.NTRSelectionMethodType
+		param          uint64
+		fields         []openflow15.MatchField
+		propertyString string
+	}{
+		{
+			name:           "dp_hash",
+			groupId:        30,
+			mt:             openflow15.NTR_DP_HASH,
+			param:          0x1ffffffff,
+			propertyString: "selection_method=dp_hash,selection_method_param=8589934591",
+		},
+		{
+			name:           "hash without fields",
+			groupId:        31,
+			mt:             openflow15.NTR_HASH,
+			param:          0x1ffffffff,
+			propertyString: "selection_method=hash,selection_method_param=8589934591",
+		},
+		{
+			name:           "hash with fields",
+			groupId:        32,
+			mt:             openflow15.NTR_HASH,
+			fields:         []openflow15.MatchField{*tcpSrcPortFieldWithValue},
+			propertyString: "selection_method=hash,fields=tcp_src=63",
+		},
+		{
+			name:           "hash with fields no value",
+			groupId:        32,
+			mt:             openflow15.NTR_HASH,
+			fields:         []openflow15.MatchField{*tcpSrcPortFieldWithoutValue},
+			propertyString: "selection_method=hash,fields=tcp_src",
+		},
+		{
+			name:           "hash with multiple fields no value",
+			groupId:        32,
+			mt:             openflow15.NTR_HASH,
+			fields:         []openflow15.MatchField{*nwProtoFieldWithoutValue, *tcpSrcPortFieldWithValue},
+			propertyString: "selection_method=hash,fields(nw_proto,tcp_src=63)",
+		}} {
+		t.Run(tc.name, func(t *testing.T) {
+			group1 := NewGroup(tc.groupId, GroupSelect, ofActor.Switch)
+			outputAction := openflow15.NewActionOutput(10)
+			bkt := openflow15.NewBucket(0)
+			bkt.AddAction(outputAction)
+			group1.AddBuckets(bkt)
+			property := openflow15.NewNTRSelectionMethod(tc.mt, tc.param, tc.fields...)
+			group1.AddProperty(property)
+
+			err := group1.Install()
+			assert.NoError(t, err, "Failed to install group entry")
+			verifyGroup(t, brName, group1, "select", "bucket=bucket_id:0,actions=output:10", tc.propertyString, true)
+			group1.Buckets = nil
+			groupMod := group1.getGroupModMessage(openflow15.OFPGC_DELETE)
+			err = group1.Switch.Send(groupMod)
+			require.NoError(t, err, "Failed to delete group")
+			verifyGroup(t, brName, group1, "select", "bucket=bucket_id:0,actions=output:10", tc.propertyString, false)
+		})
+	}
+}
+
 func createFlow(t *testing.T, mac, ip string) *Flow {
 	srcMac1, _ := net.ParseMAC(mac)
 	srcIP1 := net.ParseIP(ip)
@@ -1341,7 +1410,7 @@ func testNewFlowActionAPIsTest12(t *testing.T) {
 	err := group1.Install()
 	assert.NoError(t, err, "Failed to install group entry")
 
-	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", true)
+	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", "", true)
 
 	// Install flow and refer to group
 	inPort8 := uint32(110)
@@ -1358,7 +1427,7 @@ func testNewFlowActionAPIsTest12(t *testing.T) {
 		"priority=100,ip,in_port=110",
 		"group:1")
 	group1.Delete()
-	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", false)
+	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", "", false)
 }
 
 func TestNewFlowActionAPIs(t *testing.T) {
@@ -2090,7 +2159,7 @@ func testNXExtensionsTest13(ofApp *OfActor, ovsBr *OvsDriver, t *testing.T) {
 	group1.AddBuckets(bkt)
 	err := group1.Install()
 	assert.NoError(t, err, "Failed to install group entry")
-	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", true)
+	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", "", true)
 
 	// Install flow and refer to group
 	inPort8 := uint32(10)
@@ -2104,7 +2173,7 @@ func testNXExtensionsTest13(ofApp *OfActor, ovsBr *OvsDriver, t *testing.T) {
 		"priority=100,ip,in_port=10",
 		"group:1")
 	group1.Delete()
-	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", false)
+	verifyGroup(t, brName, group1, "select", "bucket=bucket_id:50,actions=ct(commit,nat(src=10.0.0.240,random))", "", false)
 }
 
 func testNXExtensionsWithOFApplication(ofApp *OfActor, ovsBr *OvsDriver, t *testing.T) {
@@ -2173,11 +2242,15 @@ func verifyFlowInstallAndDelete(t *testing.T, flow *Flow, nextElem FgraphElem, b
 	assert.Falsef(t, ofctlDumpFlowMatch(br, int(tableID), matchStr, actionStr), "br: %s, target flow still found on OVS after deleting it: match: %s, actions: %s", br, matchStr, actionStr)
 }
 
-func verifyGroup(t *testing.T, br string, group *Group, groupType string, buckets string, expectExists bool) {
+func verifyGroup(t *testing.T, br string, group *Group, groupType string, buckets string, properties string, expectExists bool) {
 	// dump groups
 	groupList, err := ofctlGroupDump(br)
 	assert.NoError(t, err, "Error dumping flows")
-	groupStr := fmt.Sprintf("group_id=%d,type=%s,%s", group.ID, groupType, buckets)
+	groupStr := fmt.Sprintf("group_id=%d,type=%s", group.ID, groupType)
+	if len(properties) > 0 {
+		groupStr = fmt.Sprintf("%s,%s", groupStr, properties)
+	}
+	groupStr = fmt.Sprintf("%s,%s", groupStr, buckets)
 	found := false
 	for _, groupEntry := range groupList {
 		log.Debugf("Looking for %s in %s", groupStr, groupEntry)