From f20e08cab9b7a04253c815ecd8b483992dce828f Mon Sep 17 00:00:00 2001
From: Justin SB <justinsb@google.com>
Date: Sat, 23 Apr 2022 13:40:55 -0400
Subject: [PATCH] GCE FirewallRule: Use an explicit field for ipv4 vs ipv6

We were previously relying on the name, but the name was "fooled" by
cluster names like ipv6.example.com
---
 pkg/model/gcemodel/firewall.go               |  5 +++--
 upup/pkg/fi/cloudup/gcetasks/firewallrule.go | 18 ++++++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/pkg/model/gcemodel/firewall.go b/pkg/model/gcemodel/firewall.go
index 8ac330ec39..6ff4e66481 100644
--- a/pkg/model/gcemodel/firewall.go
+++ b/pkg/model/gcemodel/firewall.go
@@ -144,8 +144,7 @@ func (b *FirewallModelBuilder) Build(c *fi.CloudupModelBuilderContext) error {
 		if err != nil {
 			return err
 		}
-		c.AddTask(&gcetasks.FirewallRule{
-			Name:         s(b.NameForFirewallRule("pod-cidrs-to-node")),
+		b.AddFirewallRulesTasks(c, "pod-cidrs-to-node", &gcetasks.FirewallRule{
 			Lifecycle:    b.Lifecycle,
 			Network:      network,
 			SourceRanges: []string{b.Cluster.Spec.Networking.PodCIDR},
@@ -180,6 +179,7 @@ func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext
 
 	ipv4 := *rule
 	ipv4.Name = s(b.NameForFirewallRule(name))
+	ipv4.Family = gcetasks.AddressFamilyIPv4
 	ipv4.SourceRanges = ipv4SourceRanges
 	if len(ipv4.SourceRanges) == 0 {
 		// This is helpful because empty SourceRanges and SourceTags are interpreted as allow everything,
@@ -191,6 +191,7 @@ func (b *GCEModelContext) AddFirewallRulesTasks(c *fi.CloudupModelBuilderContext
 
 	ipv6 := *rule
 	ipv6.Name = s(b.NameForFirewallRule(name + "-ipv6"))
+	ipv6.Family = gcetasks.AddressFamilyIPv6
 	ipv6.SourceRanges = ipv6SourceRanges
 	if len(ipv6.SourceRanges) == 0 {
 		// We specify explicitly so the rule is in IPv6 mode
diff --git a/upup/pkg/fi/cloudup/gcetasks/firewallrule.go b/upup/pkg/fi/cloudup/gcetasks/firewallrule.go
index f209aa3853..49c41015fb 100644
--- a/upup/pkg/fi/cloudup/gcetasks/firewallrule.go
+++ b/upup/pkg/fi/cloudup/gcetasks/firewallrule.go
@@ -28,10 +28,16 @@ import (
 	"k8s.io/kops/upup/pkg/fi/cloudup/terraformWriter"
 )
 
+type AddressFamily string
+
+const AddressFamilyIPv4 AddressFamily = "ipv4"
+const AddressFamilyIPv6 AddressFamily = "ipv6"
+
 // FirewallRule represents a GCE firewall rules
 // +kops:fitask
 type FirewallRule struct {
 	Name      *string
+	Family    AddressFamily
 	Lifecycle fi.Lifecycle
 
 	Network      *Network
@@ -78,6 +84,7 @@ func (e *FirewallRule) Find(c *fi.CloudupContext) (*FirewallRule, error) {
 
 	// Ignore "system" fields
 	actual.Lifecycle = e.Lifecycle
+	actual.Family = e.Family
 
 	return actual, nil
 }
@@ -112,14 +119,21 @@ func (e *FirewallRule) Normalize(c *fi.CloudupContext) error {
 		if err != nil {
 			return fmt.Errorf("sourceRange %q is not valid: %w", sourceRange, err)
 		}
+
+		if e.Family == "" {
+			// This is our own requirement, just for consistency checking.
+			// Previous we used the name, but that was confused when the cluster name was ipv6.example.com
+			return fmt.Errorf("must set Family when using SourceRanges")
+		}
+
 		if cidr.IP.To4() != nil {
 			// IPv4
-			if strings.Contains(name, "-ipv6") {
+			if e.Family != AddressFamilyIPv4 {
 				return fmt.Errorf("ipv4 ranges should not be in a ipv6-named rule (found %s in %s)", sourceRange, name)
 			}
 		} else {
 			// IPv6
-			if !strings.Contains(name, "-ipv6") {
+			if e.Family != AddressFamilyIPv6 {
 				return fmt.Errorf("ipv6 ranges should be in a ipv6-named rule (found %s in %s)", sourceRange, name)
 			}
 		}