From 889ba832fa04c0c8edf335e57cb5e9695ddef5ca Mon Sep 17 00:00:00 2001 From: Alex Lubbock Date: Wed, 22 Apr 2026 23:11:14 +0100 Subject: [PATCH] fix(firewall): do not require --startport for ICMP rules ICMP is a portless protocol, so --startport should not be mandatory when creating firewall rules with --protocol=ICMP. Two issues were found and fixed: 1. MarkFlagRequired("startport") was enforced globally by Cobra before the command ran, making it impossible to omit regardless of protocol. Removed the declaration and replaced it with a runtime check that only errors for TCP/UDP. 2. The Civo API is case-sensitive: it only skips the port-empty validation when protocol is sent as lowercase "icmp". Sending "ICMP" triggered a 400 firewall_ports_should_not_be_empty error even though start_port and end_port were present as empty strings in the JSON body. Fixed by normalising protocol to lowercase before building the request config. EndPort assignment and success output messages are also updated to omit port information for ICMP rules. --- cmd/firewall/firewall.go | 2 -- cmd/firewall/firewall_rule_create.go | 26 +++++++++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/cmd/firewall/firewall.go b/cmd/firewall/firewall.go index bcde13e1..adaceac5 100644 --- a/cmd/firewall/firewall.go +++ b/cmd/firewall/firewall.go @@ -57,8 +57,6 @@ func init() { firewallRuleCreateCmd.Flags().StringVarP(&direction, "direction", "d", "ingress", "the direction of the rule can be ingress or egress (default is ingress)") firewallRuleCreateCmd.Flags().StringVarP(&action, "action", "a", "allow", "the action of the rule can be allow or deny (default is allow)") firewallRuleCreateCmd.Flags().StringVarP(&label, "label", "l", "", "a string that will be the displayed as the name/reference for this rule") - _ = firewallRuleCreateCmd.MarkFlagRequired("startport") - // Mark the create-rules flag as deprecated _ = firewallCreateCmd.Flags().MarkDeprecated("create-rules", "it will be removed in future versions. Default firewall rules are created by default. Use --no-default-rules flag to create firewalls without them.\n") } diff --git a/cmd/firewall/firewall_rule_create.go b/cmd/firewall/firewall_rule_create.go index 15e3ce39..8e8f825c 100644 --- a/cmd/firewall/firewall_rule_create.go +++ b/cmd/firewall/firewall_rule_create.go @@ -47,9 +47,15 @@ var firewallRuleCreateCmd = &cobra.Command{ os.Exit(1) } + isICMP := strings.EqualFold(protocol, "icmp") + if !isICMP && startPort == "" { + utility.Error("'--startport' flag is required for protocol %s", strings.ToUpper(protocol)) + os.Exit(1) + } + newRuleConfig := &civogo.FirewallRuleConfig{ FirewallID: firewall.ID, - Protocol: protocol, + Protocol: strings.ToLower(protocol), StartPort: startPort, Cidr: strings.Split(cidr, ","), Label: label, @@ -71,10 +77,12 @@ var firewallRuleCreateCmd = &cobra.Command{ os.Exit(1) } - if endPort == "" { - newRuleConfig.EndPort = startPort - } else { - newRuleConfig.EndPort = endPort + if !isICMP { + if endPort == "" { + newRuleConfig.EndPort = startPort + } else { + newRuleConfig.EndPort = endPort + } } rule, err := client.NewFirewallRule(newRuleConfig) @@ -92,13 +100,17 @@ var firewallRuleCreateCmd = &cobra.Command{ ow.WriteCustomOutput(common.OutputFields) default: if rule.Label == "" { - if newRuleConfig.EndPort == newRuleConfig.StartPort { + if isICMP { + fmt.Printf("Created a firewall rule to %s, %s access for %s %s %s with ID %s\n", utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(strings.ToUpper(newRuleConfig.Protocol)), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID) + } else if newRuleConfig.EndPort == newRuleConfig.StartPort { fmt.Printf("Created a firewall rule to %s, %s access to port %s %s %s with ID %s\n", utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(newRuleConfig.StartPort), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID) } else { fmt.Printf("Created a firewall rule to %s, %s access to ports %s-%s %s %s with ID %s\n", utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(newRuleConfig.StartPort), utility.Green(newRuleConfig.EndPort), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID) } } else { - if newRuleConfig.EndPort == newRuleConfig.StartPort { + if isICMP { + fmt.Printf("Created a firewall rule called %s to %s, %s access for %s %s %s with ID %s\n", utility.Green(rule.Label), utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(strings.ToUpper(newRuleConfig.Protocol)), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID) + } else if newRuleConfig.EndPort == newRuleConfig.StartPort { fmt.Printf("Created a firewall rule called %s to %s, %s access to port %s %s %s with ID %s\n", utility.Green(rule.Label), utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(newRuleConfig.StartPort), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID) } else { fmt.Printf("Created a firewall rule called %s to %s, %s access to ports %s-%s %s %s with ID %s\n", utility.Green(rule.Label), utility.Green(newRuleConfig.Action), utility.Green(newRuleConfig.Direction), utility.Green(newRuleConfig.StartPort), utility.Green(newRuleConfig.EndPort), directionValue, utility.Green(strings.Join(newRuleConfig.Cidr, ", ")), rule.ID)