diff --git a/.changes/v2.25.0/680-bug-fixes.md b/.changes/v2.25.0/680-bug-fixes.md new file mode 100644 index 000000000..addc99284 --- /dev/null +++ b/.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] diff --git a/govcd/nsxt_distributed_firewall.go b/govcd/nsxt_distributed_firewall.go index d5e605174..188f4dd16 100644 --- a/govcd/nsxt_distributed_firewall.go +++ b/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 } diff --git a/govcd/nsxt_distributed_firewall_test.go b/govcd/nsxt_distributed_firewall_test.go index 4af79db31..b43a383a0 100644 --- a/govcd/nsxt_distributed_firewall_test.go +++ b/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 := ""