fix(firewall): fw controls bugfixes (#287)

fix(firewall): fw controls bugfixes

- fix alias examples in docs
- docs & examples improvements
- add support for security group insertion

Release-As: 0.17.0-rc2

docs/resources/virtual_environment_cluster_firewall_security_group.md

@@ -65,9 +65,10 @@ resource "proxmox_virtual_environment_cluster_firewall_security_group" "webserve
       rules can use arbitrary strings.
     - `log` - (Optional) Log level for this rule (`emerg`, `alert`, `crit`,
       `err`, `warning`, `notice`, `info`, `debug`, `nolog`).
-    - `macro`- (Optional) Macro name. Use predefined standard macro.
+    - `macro`- (Optional) Macro name. Use predefined standard macro
+      from https://pve.proxmox.com/pve-docs/pve-admin-guide.html#_firewall_macro_definitions
     - `proto` - (Optional) Restrict packet protocol. You can use protocol names
-      or simple numbers (0-255), as defined in '/etc/protocols'.
+      as defined in '/etc/protocols'.
     - `source` - (Optional) Restrict packet source address. This can refer
       to a single IP address, an IP set ('+ipsetname') or an IP alias
       definition. You can also specify an address range like

docs/resources/virtual_environment_firewall_alias.md

@@ -1,13 +1,13 @@
 ---
 layout: page
-title: proxmox_virtual_environment_cluster_firewall_alias
+title: proxmox_virtual_environment_firewall_alias
-permalink: /resources/virtual_environment_cluster_firewall_alias
+permalink: /resources/virtual_environment_firewall_alias
 nav_order: 7
 parent: Resources
 subcategory: Virtual Environment
 ---
 
-# Resource: proxmox_virtual_environment_cluster_firewall_alias
+# Resource: proxmox_virtual_environment_firewall_alias
 
 Aliases are used to see what devices or group of devices are affected by a rule.
 We can create aliases to identify an IP address or a network. Aliases can be
@@ -16,7 +16,7 @@ created on the cluster level, on VM / Container level.
 ## Example Usage
 
 ```terraform
-resource "proxmox_virtual_environment_cluster_firewall_alias" "local_network" {
+resource "proxmox_virtual_environment_firewall_alias" "local_network" {
   depends_on = [proxmox_virtual_environment_vm.example]
 
   node_name = proxmox_virtual_environment_vm.example.node_name
@@ -27,7 +27,7 @@ resource "proxmox_virtual_environment_cluster_firewall_alias" "local_network" {
   comment = "Managed by Terraform"
 }
 
-resource "proxmox_virtual_environment_cluster_firewall_alias" "ubuntu_vm" {
+resource "proxmox_virtual_environment_firewall_alias" "ubuntu_vm" {
   name    = "ubuntu"
   cidr    = "192.168.0.1"
   comment = "Managed by Terraform"

docs/resources/virtual_environment_firewall_options.md

@@ -15,6 +15,11 @@ Manages firewall options on VM / Container level.
 
 ```terraform
 resource "proxmox_virtual_environment_firewall_options" "example" {
+  depends_on = [proxmox_virtual_environment_vm.example]
+
+  node_name = proxmox_virtual_environment_vm.example.node_name
+  vm_id     = proxmox_virtual_environment_vm.example.vm_id
+  
   enabled = false
 
   dhcp          = true

docs/resources/virtual_environment_firewall_rules.md

@@ -18,7 +18,10 @@ level, on VM / Container level.
 
 ```terraform
 resource "proxmox_virtual_environment_firewall_rules" "inbound" {
-  depends_on = [proxmox_virtual_environment_vm.example]
+  depends_on = [
+    proxmox_virtual_environment_vm.example,
+    proxmox_virtual_environment_cluster_firewall_security_group.example,
+  ]
 
   node_name = proxmox_virtual_environment_vm.example.node_name
   vm_id     = proxmox_virtual_environment_vm.example.vm_id
@@ -42,47 +45,66 @@ resource "proxmox_virtual_environment_firewall_rules" "inbound" {
     proto   = "tcp"
     log     = "info"
   }
+
+  rule {
+    security_group = proxmox_virtual_environment_cluster_firewall_security_group.example.name
+    comment        = "From security group"
+    iface          = "net0"
+  }
 }
 ```
 
 ## Argument Reference
 
-- `node_name` - (Optional) Node name. Leave empty for cluster level aliases.
+- `node_name` - (Optional) Node name. Leave empty for cluster level rules.
-- `vm_id` - (Optional) VM ID. Leave empty for cluster level aliases.
+- `vm_id` - (Optional) VM ID. Leave empty for cluster level rules.
-- `container_id` - (Optional) Container ID. Leave empty for cluster level aliases.
+- `container_id` - (Optional) Container ID. Leave empty for cluster level
+  rules.
 - `rule` - (Optional) Firewall rule block (multiple blocks supported).
+  The provider supports two types of the `rule` blocks:
+    - a rule definition block, which includes the following arguments:
         - `action` - (Required) Rule action (`ACCEPT`, `DROP`, `REJECT`).
         - `type` - (Required) Rule type (`in`, `out`).
         - `comment` - (Optional) Rule comment.
-    - `dest` - (Optional) Restrict packet destination address. This can refer to
+        - `dest` - (Optional) Restrict packet destination address. This can
-      a single IP address, an IP set ('+ipsetname') or an IP alias definition.
+          refer to a single IP address, an IP set ('+ipsetname') or an IP alias
-      You can also specify an address range like `20.34.101.207-201.3.9.99`, or
+          definition. You can also specify an address range
-      a list of IP addresses and networks (entries are separated by comma).
+          like `20.34.101.207-201.3.9.99`, or a list of IP addresses and
-      Please do not mix IPv4 and IPv6 addresses inside such lists.
+          networks (entries are separated by comma). Please do not mix IPv4 and
+          IPv6 addresses inside such lists.
         - `dport` - (Optional) Restrict TCP/UDP destination port. You can use
-      service names or simple numbers (0-65535), as defined in '/etc/services'.
+          service names or simple numbers (0-65535), as defined
-      Port ranges can be specified with '\d+:\d+', for example `80:85`, and
+          in `/etc/services`. Port ranges can be specified with '\d+:\d+', for
-      you can use comma separated list to match several ports or ranges.
+          example `80:85`, and you can use comma separated list to match several
-    - `enable` - (Optional) Enable this rule. Defaults to `true`.
+          ports or ranges.
+        - `enabled` - (Optional) Enable this rule. Defaults to `true`.
         - `iface` - (Optional) Network interface name. You have to use network
-      configuration key names for VMs and containers ('net\d+'). Host related
+          configuration key names for VMs and containers ('net\d+'). Host
-      rules can use arbitrary strings.
+          related rules can use arbitrary strings.
         - `log` - (Optional) Log level for this rule (`emerg`, `alert`, `crit`,
           `err`, `warning`, `notice`, `info`, `debug`, `nolog`).
-    - `macro`- (Optional) Macro name. Use predefined standard macro.
+        - `macro`- (Optional) Macro name. Use predefined standard macro
-    - `proto` - (Optional) Restrict packet protocol. You can use protocol names
+          from https://pve.proxmox.com/pve-docs/pve-admin-guide.html#_firewall_macro_definitions
-      or simple numbers (0-255), as defined in '/etc/protocols'.
+        - `proto` - (Optional) Restrict packet protocol. You can use protocol
+          names as defined in '/etc/protocols'.
         - `source` - (Optional) Restrict packet source address. This can refer
           to a single IP address, an IP set ('+ipsetname') or an IP alias
-      definition. You can also specify an address range like
+          definition. You can also specify an address range
-      `20.34.101.207-201.3.9.99`, or a list of IP addresses and networks (
+          like `20.34.101.207-201.3.9.99`, or a list of IP addresses and
-      entries
+          networks (entries are separated by comma). Please do not mix IPv4 and
-      are separated by comma). Please do not mix IPv4 and IPv6 addresses inside
+          IPv6 addresses inside such lists.
-      such lists.
         - `sport` - (Optional) Restrict TCP/UDP source port. You can use
-      service names or simple numbers (0-65535), as defined in '/etc/services'.
+          service names or simple numbers (0-65535), as defined
-      Port ranges can be specified with '\d+:\d+', for example `80:85`, and
+          in `/etc/services`. Port ranges can be specified with '\d+:\d+', for
-      you can use comma separated list to match several ports or ranges.
+          example `80:85`, and you can use comma separated list to match several
+          ports or ranges.
+    - a security group insertion block, which includes the following arguments:
+        - `comment` - (Optional) Rule comment.
+        - `enabled` - (Optional) Enable this rule. Defaults to `true`.
+        - `iface` - (Optional) Network interface name. You have to use network
+          configuration key names for VMs and containers ('net\d+'). Host
+          related rules can use arbitrary strings.
+        - `security_group` - (Required) Security group name.
 
 ## Attribute Reference
 

example/resource_virtual_environment_cluster_firewall_security_group.tf

@@ -15,7 +15,7 @@ resource "proxmox_virtual_environment_cluster_firewall_security_group" "example"
   rule {
     type    = "in"
     action  = "DROP"
-    comment = "Allow SSH"
+    comment = "Drop SSH"
     dest    = "192.168.1.5"
     dport   = "22"
     proto   = "udp"

example/resource_virtual_environment_firewall_rules.tf

@@ -12,7 +12,7 @@ resource "proxmox_virtual_environment_firewall_rules" "cluster_rules" {
   rule {
     type    = "out"
     action  = "DROP"
-    comment = "Allow SSH"
+    comment = "Drop SSH"
     dest    = "192.168.0.5"
     dport   = "22"
     proto   = "tcp"
@@ -20,16 +20,26 @@ resource "proxmox_virtual_environment_firewall_rules" "cluster_rules" {
 }
 
 resource "proxmox_virtual_environment_firewall_rules" "vm_rules" {
-  depends_on = [proxmox_virtual_environment_vm.example]
+  depends_on = [
+    proxmox_virtual_environment_vm.example,
+    proxmox_virtual_environment_cluster_firewall_security_group.example,
+  ]
 
   node_name = proxmox_virtual_environment_vm.example.node_name
   vm_id     = proxmox_virtual_environment_vm.example.vm_id
 
+  rule {
+    security_group = proxmox_virtual_environment_cluster_firewall_security_group.example.name
+    enabled        = true
+    comment        = "From XXX"
+    iface          = "net0"
+  }
+
   rule {
     type    = "in"
     action  = "ACCEPT"
     comment = "Allow FTP"
-    dest    = "192.168.1.5"
+    dest    = "192.168.1.15"
     dport   = "21"
     proto   = "tcp"
     log     = "info"
@@ -38,8 +48,8 @@ resource "proxmox_virtual_environment_firewall_rules" "vm_rules" {
   rule {
     type    = "out"
     action  = "DROP"
-    comment = "Allow SSH"
+    comment = "Drop SSH"
-    dest    = "192.168.1.5"
+    dest    = "192.168.1.15"
     dport   = "22"
     proto   = "tcp"
   }
@@ -64,7 +74,7 @@ resource "proxmox_virtual_environment_firewall_rules" "container_rules" {
   rule {
     type    = "out"
     action  = "DROP"
-    comment = "Allow SSH"
+    comment = "Drop SSH"
     dest    = "192.168.2.5"
     dport   = "22"
     proto   = "tcp"

proxmoxtf/resource/firewall/rules.go

@@ -22,12 +22,13 @@ import (
 )
 
 const (
+	dvSecurityGroup = ""
 	dvRuleComment   = ""
 	dvRuleDPort     = ""
 	dvRuleDest      = ""
-	dvRuleEnable  = true
+	dvRuleEnabled   = true
 	dvRuleIface     = ""
-	dvRuleLog     = "nolog"
+	dvRuleLog       = ""
 	dvRuleMacro     = ""
 	dvRuleProto     = ""
 	dvRuleSPort     = ""
@@ -35,11 +36,13 @@ const (
 
 	MkRule = "rule"
 
+	mkSecurityGroup = "security_group"
+
 	mkRuleAction  = "action"
 	mkRuleComment = "comment"
 	mkRuleDPort   = "dport"
 	mkRuleDest    = "dest"
-	mkRuleEnable  = "enable"
+	mkRuleEnabled = "enabled"
 	mkRuleIFace   = "iface"
 	mkRuleLog     = "log"
 	mkRuleMacro   = "macro"
@@ -57,16 +60,23 @@ func Rules() *schema.Resource {
 			Description: "Rules position",
 			Computed:    true,
 		},
+		mkSecurityGroup: {
+			Type:        schema.TypeString,
+			Description: "Security group name",
+			Optional:    true,
+			ForceNew:    true,
+			Default:     dvSecurityGroup,
+		},
 		mkRuleAction: {
 			Type:             schema.TypeString,
 			Description:      "Rules action ('ACCEPT', 'DROP', 'REJECT')",
-			Required:         true,
+			Optional:         true,
 			ValidateDiagFunc: validator.FirewallPolicy(),
 		},
 		mkRuleType: {
 			Type:             schema.TypeString,
 			Description:      "Rules type ('in', 'out')",
-			Required:         true,
+			Optional:         true,
 			ValidateDiagFunc: validation.ToDiagFunc(validation.StringInSlice([]string{"in", "out"}, true)),
 		},
 		mkRuleComment: {
@@ -92,11 +102,11 @@ func Rules() *schema.Resource {
 			Optional: true,
 			Default:  dvRuleDPort,
 		},
-		mkRuleEnable: {
+		mkRuleEnabled: {
 			Type:        schema.TypeBool,
 			Description: "Enable rule",
 			Optional:    true,
-			Default:     dvRuleEnable,
+			Default:     dvRuleEnabled,
 		},
 		mkRuleIFace: {
 			Type: schema.TypeString,
@@ -104,6 +114,7 @@ func Rules() *schema.Resource {
 				" and containers ('net\\d+'). Host related rules can use arbitrary strings.",
 			Optional:         true,
 			Default:          dvRuleIface,
+			ValidateDiagFunc: validator.FirewallIFace(),
 		},
 		mkRuleLog: {
 			Type: schema.TypeString,
@@ -171,11 +182,29 @@ func RulesCreate(ctx context.Context, api firewall.Rule, d *schema.ResourceData)
 	rules := d.Get(MkRule).([]interface{})
 	for i := len(rules) - 1; i >= 0; i-- {
 		rule := rules[i].(map[string]interface{})
+		var ruleBody firewall.RuleCreateRequestBody
 
-		ruleBody := firewall.RuleCreateRequestBody{
+		sg := rule[mkSecurityGroup].(string)
+		if sg != "" {
+			// this is a special case of security group insertion
+			ruleBody = firewall.RuleCreateRequestBody{
+				Action:   sg,
+				Type:     "group",
+				BaseRule: *mapToSecurityGroupBaseRule(rule),
+			}
+		} else {
+			a := rule[mkRuleAction].(string)
+			t := rule[mkRuleType].(string)
+			if a == "" || t == "" {
+				diags = append(diags, diag.Errorf("Either '%s' OR both '%s' and '%s' must be defined for the rule #%d",
+					mkSecurityGroup, mkRuleAction, mkRuleType, i)...)
+				continue
+			}
+			ruleBody = firewall.RuleCreateRequestBody{
+				Action:   a,
+				Type:     t,
 				BaseRule: *mapToBaseRule(rule),
-			Action:   rule[mkRuleAction].(string),
+			}
-			Type:     rule[mkRuleType].(string),
 		}
 
 		err := api.CreateRule(ctx, &ruleBody)
@@ -206,12 +235,18 @@ func RulesRead(ctx context.Context, api firewall.Rule, d *schema.ResourceData) d
 			return fmt.Errorf("error reading rule %d : %w", pos, err)
 		}
 
-		baseRuleToMap(&rule.BaseRule, ruleMap)
-
 		// 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
 	}
@@ -324,11 +359,9 @@ func mapToBaseRule(rule map[string]interface{}) *firewall.BaseRule {
 	if dport != "" {
 		baseRule.DPort = &dport
 	}
-	enable := rule[mkRuleEnable].(bool)
+	enableBool := types.CustomBool(rule[mkRuleEnabled].(bool))
-	if enable {
-		enableBool := types.CustomBool(true)
 	baseRule.Enable = &enableBool
-	}
+
 	iface := rule[mkRuleIFace].(string)
 	if iface != "" {
 		baseRule.IFace = &iface
@@ -356,6 +389,24 @@ func mapToBaseRule(rule map[string]interface{}) *firewall.BaseRule {
 
 	return baseRule
 }
+func mapToSecurityGroupBaseRule(rule map[string]interface{}) *firewall.BaseRule {
+	baseRule := &firewall.BaseRule{}
+
+	comment := rule[mkRuleComment].(string)
+	if comment != "" {
+		baseRule.Comment = &comment
+	}
+
+	enableBool := types.CustomBool(rule[mkRuleEnabled].(bool))
+	baseRule.Enable = &enableBool
+
+	iface := rule[mkRuleIFace].(string)
+	if iface != "" {
+		baseRule.IFace = &iface
+	}
+
+	return baseRule
+}
 
 func baseRuleToMap(baseRule *firewall.BaseRule, rule map[string]interface{}) {
 	if baseRule.Comment != nil {
@@ -368,7 +419,7 @@ func baseRuleToMap(baseRule *firewall.BaseRule, rule map[string]interface{}) {
 		rule[mkRuleDPort] = *baseRule.DPort
 	}
 	if baseRule.Enable != nil {
-		rule[mkRuleEnable] = *baseRule.Enable
+		rule[mkRuleEnabled] = *baseRule.Enable
 	}
 	if baseRule.IFace != nil {
 		rule[mkRuleIFace] = *baseRule.IFace
@@ -389,6 +440,17 @@ func baseRuleToMap(baseRule *firewall.BaseRule, rule map[string]interface{}) {
 		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,

proxmoxtf/resource/firewall/rules_test.go

@@ -37,16 +37,14 @@ func TestRuleSchema(t *testing.T) {
 
 	ruleSchema := structure.AssertNestedSchemaExistence(t, rulesSchema, MkRule).Schema
 
-	structure.AssertRequiredArguments(t, ruleSchema, []string{
+	structure.AssertOptionalArguments(t, ruleSchema, []string{
+		mkSecurityGroup,
 		mkRuleAction,
 		mkRuleType,
-	})
-
-	structure.AssertOptionalArguments(t, ruleSchema, []string{
 		mkRuleComment,
 		mkRuleDest,
 		mkRuleDPort,
-		mkRuleEnable,
+		mkRuleEnabled,
 		mkRuleIFace,
 		mkRuleLog,
 		mkRuleMacro,
@@ -62,7 +60,7 @@ func TestRuleSchema(t *testing.T) {
 		mkRuleComment: schema.TypeString,
 		mkRuleDest:    schema.TypeString,
 		mkRuleDPort:   schema.TypeString,
-		mkRuleEnable:  schema.TypeBool,
+		mkRuleEnabled: schema.TypeBool,
 		mkRuleIFace:   schema.TypeString,
 		mkRuleLog:     schema.TypeString,
 		mkRuleMacro:   schema.TypeString,

proxmoxtf/resource/validator/firewall.go

@@ -13,7 +13,10 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 )
 
-var rateExpression = regexp.MustCompile(`[1-9][0-9]*/(second|minute|hour|day)`)
+var (
+	rateExpression  = regexp.MustCompile(`[1-9][0-9]*/(second|minute|hour|day)`)
+	ifaceExpression = regexp.MustCompile(`net\d+`)
+)
 
 func FirewallRate() schema.SchemaValidateDiagFunc {
 	return validation.ToDiagFunc(validation.StringMatch(
@@ -21,6 +24,12 @@ func FirewallRate() schema.SchemaValidateDiagFunc {
 		"Must be a valid rate expression, e.g. '1/second'",
 	))
 }
+func FirewallIFace() schema.SchemaValidateDiagFunc {
+	return validation.ToDiagFunc(validation.StringMatch(
+		ifaceExpression,
+		"Must be a valid VM/Container iface key, e.g. 'net0'",
+	))
+}
 
 func FirewallPolicy() schema.SchemaValidateDiagFunc {
 	return validation.ToDiagFunc(validation.StringInSlice(