From 8ac7ec25f0e13e838d9e067a5b5c2db8b2ec5ccd Mon Sep 17 00:00:00 2001 From: Anna Khmelnitsky Date: Wed, 7 Sep 2022 21:15:22 +0000 Subject: [PATCH] Update FW policy rules only if needed In security policy and gateway policy resources, any change would trigger an update for the whole list of rules. This change switches to use H-API, and would update rules on individual basis, and rule will be updated only if terraform detects a change in it. This will prevent needless reset of statistics for untouched rules. Please note that if a rule is deleted, terraform would detect a change in all subsequent rules, since rules are considered an ordered list. User can consider disabling rule as an alternative. Signed-off-by: Anna Khmelnitsky --- nsxt/policy_common.go | 15 +- nsxt/resource_nsxt_policy_gateway_policy.go | 151 +++++++++++------- ...e_nsxt_policy_predefined_gateway_policy.go | 2 +- ..._nsxt_policy_predefined_security_policy.go | 2 +- nsxt/resource_nsxt_policy_security_policy.go | 99 ++++-------- 5 files changed, 129 insertions(+), 140 deletions(-) diff --git a/nsxt/policy_common.go b/nsxt/policy_common.go index 74b4ad73c..9bc6627ec 100644 --- a/nsxt/policy_common.go +++ b/nsxt/policy_common.go @@ -390,7 +390,7 @@ func setPolicyRulesInSchema(d *schema.ResourceData, rules []model.Rule) error { return d.Set("rule", rulesList) } -func getPolicyRulesFromSchema(d *schema.ResourceData, setNsxID bool) []model.Rule { +func getPolicyRulesFromSchema(d *schema.ResourceData) []model.Rule { rules := d.Get("rule").([]interface{}) var ruleList []model.Rule seq := 0 @@ -410,9 +410,11 @@ func getPolicyRulesFromSchema(d *schema.ResourceData, setNsxID bool) []model.Rul sequenceNumber := int64(seq) tagStructs := getPolicyTagsFromSet(data["tag"].(*schema.Set)) - // Use a different random Id each time, otherwise Update requires revision - // to be set for existing rules, and NOT be set for new rules id := newUUID() + nsxID := data["nsx_id"].(string) + if nsxID != "" { + id = nsxID + } resourceType := "Rule" elem := model.Rule{ @@ -438,13 +440,6 @@ func getPolicyRulesFromSchema(d *schema.ResourceData, setNsxID bool) []model.Rul SequenceNumber: &sequenceNumber, } - if setNsxID { - nsxID := data["nsx_id"].(string) - if nsxID != "" { - elem.Id = &nsxID - } - } - ruleList = append(ruleList, elem) seq = seq + 1 } diff --git a/nsxt/resource_nsxt_policy_gateway_policy.go b/nsxt/resource_nsxt_policy_gateway_policy.go index 37606d7b5..4136c918a 100644 --- a/nsxt/resource_nsxt_policy_gateway_policy.go +++ b/nsxt/resource_nsxt_policy_gateway_policy.go @@ -8,6 +8,7 @@ import ( "log" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/vmware/vsphere-automation-sdk-go/runtime/data" "github.com/vmware/vsphere-automation-sdk-go/runtime/protocol/client" gm_domains "github.com/vmware/vsphere-automation-sdk-go/services/nsxt-gm/global_infra/domains" gm_model "github.com/vmware/vsphere-automation-sdk-go/services/nsxt-gm/model" @@ -67,15 +68,67 @@ func resourceNsxtPolicyGatewayPolicyExistsPartial(domainName string) func(id str } } -func resourceNsxtPolicyGatewayPolicyCreate(d *schema.ResourceData, m interface{}) error { - connector := getPolicyConnector(m) +func getUpdatedRuleChildren(d *schema.ResourceData) ([]*data.StructValue, error) { + var policyChildren []*data.StructValue - // Initialize resource Id and verify this ID is not yet used - id, err := getOrGenerateID(d, m, resourceNsxtPolicyGatewayPolicyExistsPartial(d.Get("domain").(string))) - if err != nil { - return err + if !d.HasChange("rule") { + return nil, nil } + oldRules, newRules := d.GetChange("rule") + rules := getPolicyRulesFromSchema(d) + newRulesCount := len(newRules.([]interface{})) + oldRulesCount := len(oldRules.([]interface{})) + for ruleNo := 0; ruleNo < newRulesCount; ruleNo++ { + ruleIndicator := fmt.Sprintf("rule.%d", ruleNo) + if d.HasChange(ruleIndicator) { + rule := rules[ruleNo] + // New or updated rule + ruleID := newUUID() + if rule.Id != nil { + ruleID = *rule.Id + log.Printf("[DEBUG]: Updating child rule with id %s", *rule.Id) + } else { + log.Printf("[DEBUG]: Adding child rule with id %s", ruleID) + rule.Id = &ruleID + } + + childRule, err := createPolicyChildRule(ruleID, rule, false) + if err != nil { + return policyChildren, err + } + policyChildren = append(policyChildren, childRule) + } + } + + resourceType := "Rule" + for ruleNo := newRulesCount; ruleNo < oldRulesCount; ruleNo++ { + // delete + ruleIndicator := fmt.Sprintf("rule.%d", ruleNo) + oldRule, _ := d.GetChange(ruleIndicator) + oldRuleMap := oldRule.(map[string]interface{}) + oldRuleID := oldRuleMap["nsx_id"].(string) + rule := model.Rule{ + Id: &oldRuleID, + ResourceType: &resourceType, + } + + childRule, err := createPolicyChildRule(oldRuleID, rule, true) + if err != nil { + return policyChildren, err + } + log.Printf("[DEBUG]: Deleting child rule with id %s", oldRuleID) + policyChildren = append(policyChildren, childRule) + + } + + return policyChildren, nil + +} + +func policyGatewayPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, connector *client.RestConnector, isGlobalManager bool, id string) error { + + domain := d.Get("domain").(string) displayName := d.Get("display_name").(string) description := d.Get("description").(string) tags := getPolicyTagsFromSchema(d) @@ -84,7 +137,8 @@ func resourceNsxtPolicyGatewayPolicyCreate(d *schema.ResourceData, m interface{} locked := d.Get("locked").(bool) sequenceNumber := int64(d.Get("sequence_number").(int)) stateful := d.Get("stateful").(bool) - rules := getPolicyRulesFromSchema(d, false) + revision := int64(d.Get("revision").(int)) + objType := "GatewayPolicy" obj := model.GatewayPolicy{ DisplayName: &displayName, @@ -95,27 +149,41 @@ func resourceNsxtPolicyGatewayPolicyCreate(d *schema.ResourceData, m interface{} Locked: &locked, SequenceNumber: &sequenceNumber, Stateful: &stateful, - Rules: rules, + ResourceType: &objType, + Id: &id, } - _, isSet := d.GetOkExists("tcp_strict") if isSet { tcpStrict := d.Get("tcp_strict").(bool) obj.TcpStrict = &tcpStrict } - log.Printf("[INFO] Creating Gateway Policy with ID %s", id) - if isPolicyGlobalManager(m) { - client := gm_domains.NewGatewayPoliciesClient(connector) - gmObj, err1 := convertModelBindingType(obj, model.GatewayPolicyBindingType(), gm_model.GatewayPolicyBindingType()) - if err1 != nil { - return err1 - } - err = client.Patch(d.Get("domain").(string), id, gmObj.(gm_model.GatewayPolicy)) - } else { - client := domains.NewGatewayPoliciesClient(connector) - err = client.Patch(d.Get("domain").(string), id, obj) + if len(d.Id()) > 0 { + // This is update flow + obj.Revision = &revision } + + policyChildren, err := getUpdatedRuleChildren(d) + if err != nil { + return err + } + if len(policyChildren) > 0 { + obj.Children = policyChildren + } + + return gatewayPolicyInfraPatch(obj, domain, m) +} + +func resourceNsxtPolicyGatewayPolicyCreate(d *schema.ResourceData, m interface{}) error { + connector := getPolicyConnector(m) + + // Initialize resource Id and verify this ID is not yet used + id, err := getOrGenerateID(d, m, resourceNsxtPolicyGatewayPolicyExistsPartial(d.Get("domain").(string))) + if err != nil { + return err + } + + err = policyGatewayPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) if err != nil { return handleCreateError("Gateway Policy", id, err) } @@ -166,48 +234,7 @@ func resourceNsxtPolicyGatewayPolicyUpdate(d *schema.ResourceData, m interface{} return fmt.Errorf("Error obtaining Gateway Policy ID") } - displayName := d.Get("display_name").(string) - description := d.Get("description").(string) - tags := getPolicyTagsFromSchema(d) - category := d.Get("category").(string) - comments := d.Get("comments").(string) - locked := d.Get("locked").(bool) - sequenceNumber := int64(d.Get("sequence_number").(int)) - stateful := d.Get("stateful").(bool) - tcpStrict := d.Get("tcp_strict").(bool) - rules := getPolicyRulesFromSchema(d, false) - revision := int64(d.Get("revision").(int)) - - obj := model.GatewayPolicy{ - DisplayName: &displayName, - Description: &description, - Tags: tags, - Category: &category, - Comments: &comments, - Locked: &locked, - SequenceNumber: &sequenceNumber, - Stateful: &stateful, - TcpStrict: &tcpStrict, - Revision: &revision, - Rules: rules, - } - - var err error - if isPolicyGlobalManager(m) { - rawObj, err1 := convertModelBindingType(obj, model.GatewayPolicyBindingType(), gm_model.GatewayPolicyBindingType()) - if err1 != nil { - return err1 - } - gmObj := rawObj.(gm_model.GatewayPolicy) - client := gm_domains.NewGatewayPoliciesClient(connector) - // We need to use PUT, because PATCH will not replace the whole rule list - _, err = client.Update(d.Get("domain").(string), id, gmObj) - } else { - client := domains.NewGatewayPoliciesClient(connector) - // We need to use PUT, because PATCH will not replace the whole rule list - _, err = client.Update(d.Get("domain").(string), id, obj) - } - + err := policyGatewayPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) if err != nil { return handleUpdateError("Gateway Policy", id, err) } diff --git a/nsxt/resource_nsxt_policy_predefined_gateway_policy.go b/nsxt/resource_nsxt_policy_predefined_gateway_policy.go index bf1cb153c..8ddd027e8 100644 --- a/nsxt/resource_nsxt_policy_predefined_gateway_policy.go +++ b/nsxt/resource_nsxt_policy_predefined_gateway_policy.go @@ -310,7 +310,7 @@ func updatePolicyPredefinedGatewayPolicy(id string, d *schema.ResourceData, m in var childRules []*data.StructValue if d.HasChange("rule") { oldRules, _ := d.GetChange("rule") - rules := getPolicyRulesFromSchema(d, true) + rules := getPolicyRulesFromSchema(d) existingRules := make(map[string]bool) for _, rule := range rules { diff --git a/nsxt/resource_nsxt_policy_predefined_security_policy.go b/nsxt/resource_nsxt_policy_predefined_security_policy.go index 0a091833c..cbdaaa66c 100644 --- a/nsxt/resource_nsxt_policy_predefined_security_policy.go +++ b/nsxt/resource_nsxt_policy_predefined_security_policy.go @@ -235,7 +235,7 @@ func updatePolicyPredefinedSecurityPolicy(id string, d *schema.ResourceData, m i var childRules []*data.StructValue if d.HasChange("rule") { oldRules, _ := d.GetChange("rule") - rules := getPolicyRulesFromSchema(d, true) + rules := getPolicyRulesFromSchema(d) existingRules := make(map[string]bool) for _, rule := range rules { diff --git a/nsxt/resource_nsxt_policy_security_policy.go b/nsxt/resource_nsxt_policy_security_policy.go index 7437040cc..8a4e6724e 100644 --- a/nsxt/resource_nsxt_policy_security_policy.go +++ b/nsxt/resource_nsxt_policy_security_policy.go @@ -73,15 +73,9 @@ func resourceNsxtPolicySecurityPolicyExistsPartial(domainName string) func(id st } } -func resourceNsxtPolicySecurityPolicyCreate(d *schema.ResourceData, m interface{}) error { - connector := getPolicyConnector(m) - - // Initialize resource Id and verify this ID is not yet used - id, err := getOrGenerateID(d, m, resourceNsxtPolicySecurityPolicyExistsPartial(d.Get("domain").(string))) - if err != nil { - return err - } +func policySecurityPolicyBuildAndPatch(d *schema.ResourceData, m interface{}, connector *client.RestConnector, isGlobalManager bool, id string) error { + domain := d.Get("domain").(string) displayName := d.Get("display_name").(string) description := d.Get("description").(string) tags := getPolicyTagsFromSchema(d) @@ -92,9 +86,11 @@ func resourceNsxtPolicySecurityPolicyCreate(d *schema.ResourceData, m interface{ sequenceNumber := int64(d.Get("sequence_number").(int)) stateful := d.Get("stateful").(bool) tcpStrict := d.Get("tcp_strict").(bool) - rules := getPolicyRulesFromSchema(d, false) + revision := int64(d.Get("revision").(int)) + objType := "SecurityPolicy" obj := model.SecurityPolicy{ + Id: &id, DisplayName: &displayName, Description: &description, Tags: tags, @@ -105,21 +101,37 @@ func resourceNsxtPolicySecurityPolicyCreate(d *schema.ResourceData, m interface{ SequenceNumber: &sequenceNumber, Stateful: &stateful, TcpStrict: &tcpStrict, - Rules: rules, + ResourceType: &objType, } log.Printf("[INFO] Creating Security Policy with ID %s", id) - if isPolicyGlobalManager(m) { - gmObj, err1 := convertModelBindingType(obj, model.SecurityPolicyBindingType(), gm_model.SecurityPolicyBindingType()) - if err1 != nil { - return err1 - } - client := gm_domains.NewSecurityPoliciesClient(connector) - err = client.Patch(d.Get("domain").(string), id, gmObj.(gm_model.SecurityPolicy)) - } else { - client := domains.NewSecurityPoliciesClient(connector) - err = client.Patch(d.Get("domain").(string), id, obj) + + if len(d.Id()) > 0 { + // This is update flow + obj.Revision = &revision + } + + policyChildren, err := getUpdatedRuleChildren(d) + if err != nil { + return err + } + if len(policyChildren) > 0 { + obj.Children = policyChildren } + return securityPolicyInfraPatch(obj, domain, m) +} + +func resourceNsxtPolicySecurityPolicyCreate(d *schema.ResourceData, m interface{}) error { + connector := getPolicyConnector(m) + + // Initialize resource Id and verify this ID is not yet used + id, err := getOrGenerateID(d, m, resourceNsxtPolicySecurityPolicyExistsPartial(d.Get("domain").(string))) + if err != nil { + return err + } + + err = policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) + if err != nil { return handleCreateError("Security Policy", id, err) } @@ -185,52 +197,7 @@ func resourceNsxtPolicySecurityPolicyUpdate(d *schema.ResourceData, m interface{ if id == "" { return fmt.Errorf("Error obtaining Security Policy id") } - - displayName := d.Get("display_name").(string) - description := d.Get("description").(string) - tags := getPolicyTagsFromSchema(d) - category := d.Get("category").(string) - comments := d.Get("comments").(string) - locked := d.Get("locked").(bool) - scope := getStringListFromSchemaSet(d, "scope") - sequenceNumber := int64(d.Get("sequence_number").(int)) - stateful := d.Get("stateful").(bool) - tcpStrict := d.Get("tcp_strict").(bool) - rules := getPolicyRulesFromSchema(d, false) - revision := int64(d.Get("revision").(int)) - - obj := model.SecurityPolicy{ - DisplayName: &displayName, - Description: &description, - Tags: tags, - Category: &category, - Comments: &comments, - Locked: &locked, - Scope: scope, - SequenceNumber: &sequenceNumber, - Stateful: &stateful, - TcpStrict: &tcpStrict, - Revision: &revision, - Rules: rules, - } - - var err error - if isPolicyGlobalManager(m) { - gmObj, err1 := convertModelBindingType(obj, model.SecurityPolicyBindingType(), gm_model.SecurityPolicyBindingType()) - if err1 != nil { - return err1 - } - gmSecurityPolicy := gmObj.(gm_model.SecurityPolicy) - client := gm_domains.NewSecurityPoliciesClient(connector) - - // We need to use PUT, because PATCH will not replace the whole rule list - _, err = client.Update(d.Get("domain").(string), id, gmSecurityPolicy) - } else { - client := domains.NewSecurityPoliciesClient(connector) - - // We need to use PUT, because PATCH will not replace the whole rule list - _, err = client.Update(d.Get("domain").(string), id, obj) - } + err := policySecurityPolicyBuildAndPatch(d, m, connector, isPolicyGlobalManager(m), id) if err != nil { return handleUpdateError("Security Policy", id, err) }