Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency #1510

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

fix(firewall/rules): update firewall logic to handle unique rule ids, rules and terraform state with consistency #1510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee88427
fix(firewall/rules) - Update GetRulesID to provide unique ID every ti…
akazantzidis Aug 24, 2024
6dff6bb
fix(firewall/rules) - Update RulesRead to produce consistent terrafor…
akazantzidis Aug 24, 2024
1a01a96
chore(firewall/rules) - Revert prefix produced by GetRulesID
akazantzidis Aug 24, 2024
fbcd933
fix(firewall/rules): Fix typo in GetRulesID
akazantzidis Aug 25, 2024
aab5809
Merge branch 'main' into fix_firewall_rules_resources
akazantzidis Aug 26, 2024
6c210b9
Merge branch 'main' into fix_firewall_rules_resources
akazantzidis Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions proxmox/firewall/rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ import (
"context"
"fmt"
"net/http"
"strconv"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"time"

"github.com/bpg/terraform-provider-proxmox/proxmox/api"
"github.com/bpg/terraform-provider-proxmox/proxmox/types"
"github.com/google/uuid"
)

// Rule is an interface for the Proxmox firewall rule API.
Expand Down Expand Up @@ -54,7 +53,12 @@ func (c *Client) rulePath(pos int) string {

// GetRulesID returns the ID of the rules object.
func (c *Client) GetRulesID() string {
return "rule-" + strconv.Itoa(schema.HashString(c.rulesPath()))
// Creates unique id in every being called by using unix timestamp
timestamp := time.Now().UnixNano() / int64(time.Microsecond)
id := uuid.New().ID()
finalID := timestamp + int64(id)

return fmt.Sprintf("rule-%d", finalID)
}

// CreateRule creates a firewall rule.
Expand Down
162 changes: 26 additions & 136 deletions proxmoxtf/resource/firewall/rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ package firewall

import (
"context"
"fmt"
"sort"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand All @@ -34,25 +32,22 @@ const (
dvRuleProto = ""
dvRuleSPort = ""
dvRuleSource = ""

// MkRule defines the name of the rule resource in the schema.
MkRule = "rule"

MkRule = "rule"
mkSecurityGroup = "security_group"

mkRuleAction = "action"
mkRuleComment = "comment"
mkRuleDPort = "dport"
mkRuleDest = "dest"
mkRuleEnabled = "enabled"
mkRuleIFace = "iface"
mkRuleLog = "log"
mkRuleMacro = "macro"
mkRulePos = "pos"
mkRuleProto = "proto"
mkRuleSource = "source"
mkRuleSPort = "sport"
mkRuleType = "type"
mkRuleAction = "action"
mkRuleComment = "comment"
mkRuleDPort = "dport"
mkRuleDest = "dest"
mkRuleEnabled = "enabled"
mkRuleIFace = "iface"
mkRuleLog = "log"
mkRuleMacro = "macro"
mkRulePos = "pos"
mkRuleProto = "proto"
mkRuleSource = "source"
mkRuleSPort = "sport"
mkRuleType = "type"
)

// Rules returns a resource that manages firewall rules.
Expand Down Expand Up @@ -222,81 +217,32 @@ func RulesCreate(ctx context.Context, api firewall.Rule, d *schema.ResourceData)
return diags
}

// reset rules, we re-read them (with proper positions) from the API
err := d.Set(MkRule, nil)
if err != nil {
return diag.FromErr(err)
}

d.SetId(api.GetRulesID())

return RulesRead(ctx, api, d)
}

// RulesRead reads rules from the API and updates the state.
// RulesRead gets updates the tf state.
//
//nolint:revive,unused-parameter //bypass unused-parameter warning as ctx is needed for invokeRuleAPI.
func RulesRead(ctx context.Context, api firewall.Rule, d *schema.ResourceData) diag.Diagnostics {
var diags diag.Diagnostics

readRule := func(pos int, ruleMap map[string]interface{}) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. I didn't quite get this change. The RulesRead supposed to read rules from PVE API. Now it reads back only from the state 🤔

Copy link
Author

@akazantzidis akazantzidis Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sure let me explain:

rulesCreate function was resetting the rule state before calling RulesRead. This was redundant because the rules were already created in specific order in one take within rulesCreate and represented correctly in d. However, the correct pos value for each rule was not being set at that time.

By removing the state reset at the end of rulesCreate, rules are represented in the correct (in terms of user configuration) order.
Calling RulesRead we can iterate over the rules and for each rule id increment the pos value for each rule to set the actual rule pos.
This sets the pos attribute, and the in the correct order (according to user specified configuration),without reading them from the API.

Copy link
Author

@akazantzidis akazantzidis Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this proposal does not handle changes outside of Terraform, does stabilizes the terraform plan / apply / delete.
Next step though is the rule comparison in terms of API/state-configuration ,so outside changes to be reverted and to be ensured the terraform configuration.

rule, err := api.GetRule(ctx, pos)
if err != nil {
if strings.Contains(err.Error(), "no rule at position") {
// this is not an error, the rule does not exist
return nil
}

return fmt.Errorf("error reading rule %d : %w", pos, err)
}

// pos in the map should be int!
ruleMap[mkRulePos] = pos

if rule.Type == "group" {
// this is a special case of security group insertion
ruleMap[mkSecurityGroup] = rule.Action
securityGroupBaseRuleToMap(&rule.BaseRule, ruleMap)
} else {
ruleMap[mkRuleAction] = rule.Action
ruleMap[mkRuleType] = rule.Type
baseRuleToMap(&rule.BaseRule, ruleMap)
}

return nil
}

rules := d.Get(MkRule).([]interface{})
if len(rules) > 0 {
// We have rules in the state, so we need to read them from the API
for _, v := range rules {
ruleMap := v.(map[string]interface{})
pos := ruleMap[mkRulePos].(int)

err := readRule(pos, ruleMap)
diags = append(diags, diag.FromErr(err)...)
}
} else {
ruleIDs, err := api.ListRules(ctx)
if err != nil {
return diag.FromErr(err)
}
newRules := make([]map[string]interface{}, 0)

for _, id := range ruleIDs {
ruleMap := map[string]interface{}{}
c := 0

err = readRule(id.Pos, ruleMap)
if err != nil {
diags = append(diags, diag.FromErr(err)...)
} else if len(ruleMap) > 0 {
rules = append(rules, ruleMap)
}
}
}
for _, rule := range rules {
ruleMap := rule.(map[string]interface{})
id := c
ruleMap[mkRulePos] = id

if diags.HasError() {
return diags
newRules = append(newRules, ruleMap)
c++
}

err := d.Set(MkRule, rules)
err := d.Set(MkRule, newRules)
diags = append(diags, diag.FromErr(err)...)

return diags
Expand Down Expand Up @@ -444,62 +390,6 @@ func mapToSecurityGroupBaseRule(rule map[string]interface{}) *firewall.BaseRule
return baseRule
}

func baseRuleToMap(baseRule *firewall.BaseRule, rule map[string]interface{}) {
if baseRule.Comment != nil {
rule[mkRuleComment] = *baseRule.Comment
}

if baseRule.Dest != nil {
rule[mkRuleDest] = *baseRule.Dest
}

if baseRule.DPort != nil {
rule[mkRuleDPort] = *baseRule.DPort
}

if baseRule.Enable != nil {
rule[mkRuleEnabled] = *baseRule.Enable
}

if baseRule.IFace != nil {
rule[mkRuleIFace] = *baseRule.IFace
}

if baseRule.Log != nil {
rule[mkRuleLog] = *baseRule.Log
}

if baseRule.Macro != nil {
rule[mkRuleMacro] = *baseRule.Macro
}

if baseRule.Proto != nil {
rule[mkRuleProto] = *baseRule.Proto
}

if baseRule.Source != nil {
rule[mkRuleSource] = *baseRule.Source
}

if baseRule.SPort != nil {
rule[mkRuleSPort] = *baseRule.SPort
}
}

func securityGroupBaseRuleToMap(baseRule *firewall.BaseRule, rule map[string]interface{}) {
if baseRule.Comment != nil {
rule[mkRuleComment] = *baseRule.Comment
}

if baseRule.Enable != nil {
rule[mkRuleEnabled] = *baseRule.Enable
}

if baseRule.IFace != nil {
rule[mkRuleIFace] = *baseRule.IFace
}
}

func invokeRuleAPI(
f func(context.Context, firewall.Rule, *schema.ResourceData) diag.Diagnostics,
) func(context.Context, *schema.ResourceData, interface{}) diag.Diagnostics {
Expand Down