From ec1469298581fdf3302062e28fa826ef81d50975 Mon Sep 17 00:00:00 2001 From: maciejriedl Date: Thu, 5 Sep 2024 12:06:52 +0000 Subject: [PATCH 1/3] Add autogenerated files of version v1 instead of v1beta1 --- pkg/firewalls/firewalls.go | 5 +++-- pkg/firewalls/firewalls_l7_cr.go | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/firewalls/firewalls.go b/pkg/firewalls/firewalls.go index 4b5a2aba1f..7e259909cc 100644 --- a/pkg/firewalls/firewalls.go +++ b/pkg/firewalls/firewalls.go @@ -33,7 +33,8 @@ import ( const ( // DefaultFirewallName is the name to use for firewall rules created // by an L7 controller when --firewall-rule is not used. - DefaultFirewallName = "" + DefaultFirewallName = "" + DefaultFirewallDescription = "GCE L7 firewall rule" ) // FirewallRules manages firewall rules. @@ -119,7 +120,7 @@ func (fr *FirewallRules) buildExpectedFW(nodeNames, additionalPorts, additionalR expectedFirewall := &compute.Firewall{ Name: name, - Description: "GCE L7 firewall rule", + Description: DefaultFirewallDescription, SourceRanges: ranges.UnsortedList(), Network: fr.cloud.NetworkURL(), Allowed: []*compute.FirewallAllowed{ diff --git a/pkg/firewalls/firewalls_l7_cr.go b/pkg/firewalls/firewalls_l7_cr.go index 09f20fdf12..1e05bfab81 100644 --- a/pkg/firewalls/firewalls_l7_cr.go +++ b/pkg/firewalls/firewalls_l7_cr.go @@ -137,6 +137,7 @@ func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced b ObjectMeta: metav1.ObjectMeta{ Name: name, }, + Description: DefaultFirewallDescription, Spec: gcpfirewallv1.GCPFirewallSpec{ Action: gcpfirewallv1.ActionAllow, Disabled: !enforced, From e12463200baca228fdf59a3dc1282d081e29c095 Mon Sep 17 00:00:00 2001 From: maciejriedl Date: Fri, 21 Feb 2025 13:37:46 +0000 Subject: [PATCH 2/3] Revert "Add autogenerated files of version v1 instead of v1beta1" This reverts commit ec1469298581fdf3302062e28fa826ef81d50975. --- pkg/firewalls/firewalls.go | 5 ++--- pkg/firewalls/firewalls_l7_cr.go | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/firewalls/firewalls.go b/pkg/firewalls/firewalls.go index 7e259909cc..4b5a2aba1f 100644 --- a/pkg/firewalls/firewalls.go +++ b/pkg/firewalls/firewalls.go @@ -33,8 +33,7 @@ import ( const ( // DefaultFirewallName is the name to use for firewall rules created // by an L7 controller when --firewall-rule is not used. - DefaultFirewallName = "" - DefaultFirewallDescription = "GCE L7 firewall rule" + DefaultFirewallName = "" ) // FirewallRules manages firewall rules. @@ -120,7 +119,7 @@ func (fr *FirewallRules) buildExpectedFW(nodeNames, additionalPorts, additionalR expectedFirewall := &compute.Firewall{ Name: name, - Description: DefaultFirewallDescription, + Description: "GCE L7 firewall rule", SourceRanges: ranges.UnsortedList(), Network: fr.cloud.NetworkURL(), Allowed: []*compute.FirewallAllowed{ diff --git a/pkg/firewalls/firewalls_l7_cr.go b/pkg/firewalls/firewalls_l7_cr.go index 52357ea964..e986064989 100644 --- a/pkg/firewalls/firewalls_l7_cr.go +++ b/pkg/firewalls/firewalls_l7_cr.go @@ -135,7 +135,6 @@ func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced b ObjectMeta: metav1.ObjectMeta{ Name: name, }, - Description: DefaultFirewallDescription, Spec: gcpfirewallv1.GCPFirewallSpec{ Action: gcpfirewallv1.ActionAllow, Disabled: !enforced, From ff25523507bd1afbcfe726a5aa7760976d39b2e8 Mon Sep 17 00:00:00 2001 From: maciejriedl Date: Fri, 21 Feb 2025 14:07:50 +0000 Subject: [PATCH 3/3] Fix gcpfirewalls cr creation to properly set Allow - tcp:all instead of Allow - all --- pkg/firewalls/firewalls_l7_cr.go | 9 +++++++++ pkg/firewalls/firewalls_test.go | 29 ++++++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/pkg/firewalls/firewalls_l7_cr.go b/pkg/firewalls/firewalls_l7_cr.go index e986064989..85d0ef3352 100644 --- a/pkg/firewalls/firewalls_l7_cr.go +++ b/pkg/firewalls/firewalls_l7_cr.go @@ -169,6 +169,15 @@ func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced b } protocolPorts = append(protocolPorts, protocolPort) } + + // TCP:all is the default if the ports' list is empty. + if len(protocolPorts) == 0 { + protocolPort := gcpfirewallv1.ProtocolPort{ + Protocol: gcpfirewallv1.ProtocolTCP, + } + protocolPorts = append(protocolPorts, protocolPort) + } + firewallCR.Spec.Ports = protocolPorts var src_cidrs, dst_cidrs []gcpfirewallv1.CIDR diff --git a/pkg/firewalls/firewalls_test.go b/pkg/firewalls/firewalls_test.go index 156840545e..f63b070fd3 100644 --- a/pkg/firewalls/firewalls_test.go +++ b/pkg/firewalls/firewalls_test.go @@ -130,13 +130,30 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) { func TestFirewallPoolSyncPorts(t *testing.T) { fwp := NewFakeFirewallsProvider(false, false) fwClient := firewallclient.NewSimpleClientset() - fp := NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO()) - fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), true, klog.TODO()) nodes := []string{"node-a", "node-b", "node-c"} + emptyPortRanges := make([]string, 0) + + // Verify empty ports' list + fp := NewFirewallPool(fwp, defaultNamer, srcRanges, emptyPortRanges, klog.TODO()) + fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, emptyPortRanges, true, klog.TODO()) if err := fp.Sync(nodes, nil, nil, true); err != nil { t.Fatal(err) } + verifyFirewallRule(fwp, ruleName, nodes, srcRanges, emptyPortRanges, t) + + if err := fcrp.Sync(nodes, nil, nil, true); err != nil { + t.Fatal(err) + } + verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, t) + + // Verify a preset ports' list + fp = NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO()) + fcrp = NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), true, klog.TODO()) + + if err := fp.Sync(nodes, nil, nil, true); err != nil { + t.Errorf("unexpected err when syncing firewall, err: %v", err) + } verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t) if err := fcrp.Sync(nodes, nil, nil, true); err != nil { @@ -398,6 +415,12 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, ports := sets.NewString(expectedPorts...) srcranges := sets.NewString(sourceRanges...) + // Empty ports' list would mean that all protocols are permitted + // (not only TCP) + if len(actualFW.Spec.Ports) == 0 { + t.Errorf("Empty list of allowed protocols is not permited") + } + actualPorts := sets.NewString() for _, protocolports := range actualFW.Spec.Ports { if protocolports.Protocol != "TCP" { @@ -405,7 +428,7 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, } if protocolports.EndPort != nil { actualPorts.Insert(fmt.Sprintf("%d-%d", *protocolports.StartPort, *protocolports.EndPort)) - } else { + } else if protocolports.StartPort != nil { actualPorts.Insert(fmt.Sprintf("%d", *protocolports.StartPort)) }