Fix standalone distributed firewall rule management (#680)

.changes/v2.25.0/680-bug-fixes.md

@@ -0,0 +1,2 @@
+* Patched `VdcGroup.CreateDistributedFirewallRule` method that returned incorrect single rule when
+  `optionalAboveRuleId` is specified [GH-680]

govcd/nsxt_distributed_firewall.go

@@ -167,7 +167,7 @@ func (vdcGroup *VdcGroup) CreateDistributedFirewallRule(optionalAboveRuleId stri
 		return nil, nil, err
 	}
 
-	// 2. Converting the give `rule` (*types.DistributedFirewallRule) into json.RawMessage so that
+	// 2. Converting the given `rule` (*types.DistributedFirewallRule) into json.RawMessage so that
 	// it is provided in the same format as other already retrieved rules
 	newRuleRawJson, err := firewallRuleToRawJson(rule)
 	if err != nil {
@@ -199,6 +199,7 @@ func (vdcGroup *VdcGroup) CreateDistributedFirewallRule(optionalAboveRuleId stri
 		}
 		// 3.2.2 Find index for specified 'optionalAboveRuleId' rule
 		newFwRuleSliceIndex, err := getFirewallRuleIndexById(dfwRules, optionalAboveRuleId)
+		newRuleSlicePosition = newFwRuleSliceIndex // Set rule position for returning single firewall rule
 		if err != nil {
 			return nil, nil, err
 		}

govcd/nsxt_distributed_firewall_test.go

@@ -315,10 +315,41 @@ func (vcd *TestVCD) Test_NsxtDistributedFirewallRule(check *C) {
 	}()
 
 	fmt.Println("# Running Distributed Firewall tests for single Rule")
-	test_NsxtDistributedFirewallRule(vcd, check, vdcGroup.VdcGroup.Id, vcd.client, vdc)
+	test_NsxtDistributedFirewallRule(vcd, check, vdcGroup.VdcGroup.Id, vcd.client, vdc, true)
 }
 
-func test_NsxtDistributedFirewallRule(vcd *TestVCD, check *C, vdcGroupId string, vcdClient *VCDClient, vdc *Vdc) {
+func (vcd *TestVCD) Test_NsxtDistributedFirewallWithDefaultRule(check *C) {
+	if vcd.skipAdminTests {
+		check.Skip(fmt.Sprintf(TestRequiresSysAdminPrivileges, check.TestName()))
+	}
+	skipNoNsxtConfiguration(vcd, check)
+	skipOpenApiEndpointTest(vcd, check, types.OpenApiPathVersion1_0_0+types.OpenApiEndpointEdgeGateways)
+
+	adminOrg, err := vcd.client.GetAdminOrgByName(vcd.config.VCD.Org)
+	check.Assert(adminOrg, NotNil)
+	check.Assert(err, IsNil)
+
+	nsxtExternalNetwork, err := GetExternalNetworkV2ByName(vcd.client, vcd.config.VCD.Nsxt.ExternalNetwork)
+	check.Assert(nsxtExternalNetwork, NotNil)
+	check.Assert(err, IsNil)
+
+	vdc, vdcGroup := test_CreateVdcGroup(check, adminOrg, vcd)
+	check.Assert(vdc, NotNil)
+	check.Assert(vdcGroup, NotNil)
+
+	defer func() {
+		// Cleanup
+		err = vdcGroup.Delete()
+		check.Assert(err, IsNil)
+		err = vdc.DeleteWait(true, true)
+		check.Assert(err, IsNil)
+	}()
+
+	fmt.Println("# Running Distributed Firewall tests for single Rule (with default DFW rule enabled)")
+	test_NsxtDistributedFirewallRuleAboveDefault(vcd, check, vdcGroup.VdcGroup.Id, vcd.client, vdc)
+}
+
+func test_NsxtDistributedFirewallRule(vcd *TestVCD, check *C, vdcGroupId string, vcdClient *VCDClient, vdc *Vdc, deleteDefaultFirewallRule bool) {
 	adminOrg, err := vcdClient.GetAdminOrgByName(vcd.config.VCD.Org)
 	check.Assert(adminOrg, NotNil)
 	check.Assert(err, IsNil)
@@ -348,12 +379,71 @@ func test_NsxtDistributedFirewallRule(vcd *TestVCD, check *C, vdcGroupId string,
 	randomizedFwRuleSubSet := randomizedFwRuleDefs[0:5] // taking only first 5 rules to limit time of testing
 
 	// removing default firewall rule which is created by VCD when vdcGroup.ActivateDfw() is executed
+	if deleteDefaultFirewallRule {
+		err = vdcGroup.DeleteAllDistributedFirewallRules()
+		check.Assert(err, IsNil)
+	}
+
+	// Adding firewal rules one by one and checking that each of them is placed correctly
+	testDistributedFirewallRuleSequence(check, randomizedFwRuleSubSet, vdcGroup, false)
+	testDistributedFirewallRuleSequence(check, randomizedFwRuleSubSet, vdcGroup, true)
+}
+
+func test_NsxtDistributedFirewallRuleAboveDefault(vcd *TestVCD, check *C, vdcGroupId string, vcdClient *VCDClient, vdc *Vdc) {
+	adminOrg, err := vcdClient.GetAdminOrgByName(vcd.config.VCD.Org)
+	check.Assert(adminOrg, NotNil)
+	check.Assert(err, IsNil)
+
+	vdcGroup, err := adminOrg.GetVdcGroupById(vdcGroupId)
+	check.Assert(err, IsNil)
+
+	_, err = vdcGroup.ActivateDfw()
+	check.Assert(err, IsNil)
+
+	// Prep firewall rule sample to operate with
+	randomizedFwRuleDefs, ipSet, secGroup := createDistributedFirewallDefinitions(check, vcd, vdcGroup.VdcGroup.Id, vcdClient, vdc)
+	// defer cleanup function in case something goes wrong
+	defer func() {
+		dfw, err := vdcGroup.GetDistributedFirewall()
+		check.Assert(err, IsNil)
+		err = dfw.DeleteAllRules()
+		check.Assert(err, IsNil)
+		_, err = vdcGroup.DisableDefaultPolicy()
+		check.Assert(err, IsNil)
+		err = ipSet.Delete()
+		check.Assert(err, IsNil)
+		err = secGroup.Delete()
+		check.Assert(err, IsNil)
+	}()
+
+	// Get default rule by name (this name is automatically set by VCD)
+	defaultRuleName := fmt.Sprintf("Default_VdcGroup_%s", vdcGroup.VdcGroup.Name)
+	defaultRule, err := vdcGroup.GetDistributedFirewallRuleByName(defaultRuleName)
+	check.Assert(err, IsNil)
+	check.Assert(defaultRule, NotNil)
+
+	_, rule1, err := vdcGroup.CreateDistributedFirewallRule(defaultRule.Rule.ID, randomizedFwRuleDefs[0])
+	check.Assert(err, IsNil)
+
+	_, rule2, err := vdcGroup.CreateDistributedFirewallRule(defaultRule.Rule.ID, randomizedFwRuleDefs[1])
+	check.Assert(err, IsNil)
+
+	// The order should be
+	// * rule1 (created first, inserted above default rule)
+	// * rule2 (created after rule1, inserted above default rule)
+	// * Default DFW rule
+
+	allRules, err := vdcGroup.GetDistributedFirewall()
+	check.Assert(err, IsNil)
+	check.Assert(len(allRules.DistributedFirewallRuleContainer.Values), Equals, 3)
+	check.Assert(allRules.DistributedFirewallRuleContainer.Values[0].ID, Equals, rule1.Rule.ID)
+	check.Assert(allRules.DistributedFirewallRuleContainer.Values[1].ID, Equals, rule2.Rule.ID)
+	check.Assert(allRules.DistributedFirewallRuleContainer.Values[2].ID, Equals, defaultRule.Rule.ID)
+
+	// Clean up created firewall rules for next phase
 	err = vdcGroup.DeleteAllDistributedFirewallRules()
 	check.Assert(err, IsNil)
 
-	// Adding firewal rules one by one and checking that each of them is
-	testDistributedFirewallRuleSequence(vcd, check, randomizedFwRuleSubSet, vdcGroup, false)
-	testDistributedFirewallRuleSequence(vcd, check, randomizedFwRuleSubSet, vdcGroup, true)
 }
 
 // testDistributedFirewallRuleSequence tests the following:
@@ -362,7 +452,7 @@ func test_NsxtDistributedFirewallRule(vcd *TestVCD, check *C, vdcGroupId string,
 // reverseOrder=true)
 // * check that all IDs of created firewall rules persisted during further updates (means that no
 // firewall rules were recreated during addition of new ones)
-func testDistributedFirewallRuleSequence(vcd *TestVCD, check *C, randomizedFwRuleSubSet []*types.DistributedFirewallRule, vdcGroup *VdcGroup, reverseOrder bool) {
+func testDistributedFirewallRuleSequence(check *C, randomizedFwRuleSubSet []*types.DistributedFirewallRule, vdcGroup *VdcGroup, reverseOrder bool) {
 	createdIdsFound := make(map[string]bool)
 	fmt.Printf("# Creating '%d' rules one by one (reverseOrder: %t)\n", len(randomizedFwRuleSubSet), reverseOrder)
 	previousRuleId := ""