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

Fix firewalls for L4 ILBs. #2163

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Fix firewalls for L4 ILBs. #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions pkg/loadbalancers/l4.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ func (l4 *L4) ensureIPv4NodesFirewall(nodeNames []string, ipAddress string, resu
Name: firewallName,
NodeNames: nodeNames,
L4Type: utils.ILB,
Network: l4.network,
}

err = firewalls.EnsureL4LBFirewallForNodes(l4.Service, &nodesFWRParams, l4.cloud, l4.recorder)
Expand Down
119 changes: 119 additions & 0 deletions pkg/loadbalancers/l4_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1700,6 +1700,110 @@ func TestEnsureInternalDualStackLoadBalancer(t *testing.T) {
}
}

func TestEnsureIPv4Firewall4Nodes(t *testing.T) {
t.Parallel()
fakeGCE := getFakeGCECloud(gce.DefaultTestClusterValues())
nodeNames := []string{"test-node-1"}
// create a test VM so that target tags can be found
createVMInstanceWithTag(t, fakeGCE, "test-node-1", "test-node-1")

svc := test.NewL4ILBService(false, 8080)
namer := namer_util.NewL4Namer(kubeSystemUID, nil)

networkURL := "testNet"
l4ilbParams := &L4ILBParams{
Service: svc,
Cloud: fakeGCE,
Namer: namer,
Recorder: record.NewFakeRecorder(100),
Network: network.NetworkInfo{NetworkURL: networkURL, SubnetworkURL: "testSubnet"},
}
l4 := NewL4Handler(l4ilbParams)
syncResult := &L4ILBSyncResult{
Annotations: make(map[string]string),
}

l4.ensureIPv4NodesFirewall(nodeNames, "10.0.0.7", syncResult)
if syncResult.Error != nil {
t.Fatalf("ensureIPv4NodesFirewall() error %+v", syncResult)
}

firewallName := l4.namer.L4Firewall(l4.Service.Namespace, l4.Service.Name)
firewall, err := fakeGCE.GetFirewall(firewallName)
if err != nil {
t.Fatalf("GetFirewall error %v", err)
}
if firewall.Name != firewallName {
t.Errorf("Firewall.Name invalid, want=%s, got=%s", firewallName, firewall.Name)
}
if firewall.Network != networkURL {
t.Errorf("Firewall.Network invalid, want=%s, got=%s", networkURL, firewall.Network)
}
if len(firewall.Allowed) != 1 {
t.Errorf("Firewall.Allowed len unexpected, want=1, got=%d", len(firewall.Allowed))
}
expectedAllowed := &compute.FirewallAllowed{
Ports: []string{"8080"},
IPProtocol: "tcp",
}
allowed := firewall.Allowed[0]
if diff := cmp.Diff(expectedAllowed, allowed); diff != "" {
t.Errorf("Firewall.Allowed invalid, (-want +got):\n%s", diff)
}
}

func TestEnsureIPv6Firewall4Nodes(t *testing.T) {
t.Parallel()

fakeGCE := getFakeGCECloud(gce.DefaultTestClusterValues())
// create a test VM so that target tags can be found
createVMInstanceWithTag(t, fakeGCE, "test-node-1", "test-node-1")
nodeNames := []string{"test-node-1"}

svc := test.NewL4ILBService(false, 8080)
namer := namer_util.NewL4Namer(kubeSystemUID, nil)

networkURL := "testNet"
l4ilbParams := &L4ILBParams{
Service: svc,
Cloud: fakeGCE,
Namer: namer,
Recorder: record.NewFakeRecorder(100),
Network: network.NetworkInfo{NetworkURL: networkURL, SubnetworkURL: "testSubnet"},
}
l4 := NewL4Handler(l4ilbParams)
syncResult := &L4ILBSyncResult{
Annotations: make(map[string]string),
}

l4.ensureIPv6NodesFirewall("2001:db8::ff00:42:8329", nodeNames, syncResult)
if syncResult.Error != nil {
t.Fatalf("ensureIPv6NodesFirewall() error %+v", syncResult)
}
firewallName := l4.namer.L4IPv6Firewall(l4.Service.Namespace, l4.Service.Name)
firewall, err := fakeGCE.GetFirewall(firewallName)
if err != nil {
t.Fatalf("GetFirewall error %v", err)
}
if firewall.Name != firewallName {
t.Errorf("Firewall.Name invalid, want=%s, got=%s", firewallName, firewall.Name)
}
if firewall.Network != networkURL {
t.Errorf("Firewall.Network invalid, want=%s, got=%s", networkURL, firewall.Network)
}
if len(firewall.Allowed) != 1 {
t.Errorf("Firewall.Allowed len unexpected, want=1, got=%d", len(firewall.Allowed))
}
expectedAllowed := &compute.FirewallAllowed{
Ports: []string{"8080"},
IPProtocol: "tcp",
}
allowed := firewall.Allowed[0]
if diff := cmp.Diff(expectedAllowed, allowed); diff != "" {
t.Errorf("Firewall.Allowed invalid, (-want +got):\n%s", diff)
}
}

// This is exhaustive test that checks for all possible transitions of
// - ServiceExternalTrafficPolicy
// - Protocol
Expand Down Expand Up @@ -2380,3 +2484,18 @@ func verifyILBIPv6ResourcesDeletedOnSync(l4 *L4) error {

return nil
}

func createVMInstanceWithTag(t *testing.T, fakeGCE *gce.Cloud, name, tag string) {
err := fakeGCE.Compute().Instances().Insert(context.Background(),
meta.ZonalKey(tag, "us-central1-b"),
&compute.Instance{
Name: name,
Zone: "us-central1-b",
Tags: &compute.Tags{
Items: []string{tag},
},
})
if err != nil {
t.Errorf("failed to create instance err=%v", err)
}
}
1 change: 1 addition & 0 deletions pkg/loadbalancers/l4ipv6.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ func (l4 *L4) ensureIPv6NodesFirewall(ipAddress string, nodeNames []string, resu
Name: firewallName,
NodeNames: nodeNames,
L4Type: utils.ILB,
Network: l4.network,
}

err = firewalls.EnsureL4LBFirewallForNodes(l4.Service, &ipv6nodesFWRParams, l4.cloud, l4.recorder)
Expand Down
2 changes: 2 additions & 0 deletions pkg/loadbalancers/l4netlb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress st
}

// Add firewall rule for L4 External LoadBalancer traffic to nodes
defaultNetwork := network.DefaultNetwork(l4netlb.cloud)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be not default, but also as done in L4, we should store network in l4netlb struct, and pass it here

Otherwise, I guess it will fail, if nodes are not in default network

(same for l4netlbipv6)

nodesFWRParams := firewalls.FirewallParams{
PortRanges: portRanges,
SourceRanges: sourceRanges,
Expand All @@ -274,6 +275,7 @@ func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress st
Name: firewallName,
IP: l4netlb.Service.Spec.LoadBalancerIP,
NodeNames: nodeNames,
Network: *defaultNetwork,
}
result.Error = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &nodesFWRParams, l4netlb.cloud, l4netlb.recorder)
if result.Error != nil {
Expand Down
3 changes: 3 additions & 0 deletions pkg/loadbalancers/l4netlbipv6.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package loadbalancers

import (
"k8s.io/ingress-gce/pkg/network"
"strings"
"time"

Expand Down Expand Up @@ -122,6 +123,7 @@ func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(ipAddress string, nodeNames []st
return
}

defaultNetwork := network.DefaultNetwork(l4netlb.cloud)
ipv6nodesFWRParams := firewalls.FirewallParams{
PortRanges: portRanges,
SourceRanges: ipv6SourceRanges,
Expand All @@ -130,6 +132,7 @@ func (l4netlb *L4NetLB) ensureIPv6NodesFirewall(ipAddress string, nodeNames []st
Name: firewallName,
NodeNames: nodeNames,
L4Type: utils.XLB,
Network: *defaultNetwork,
}

err = firewalls.EnsureL4LBFirewallForNodes(l4netlb.Service, &ipv6nodesFWRParams, l4netlb.cloud, l4netlb.recorder)
Expand Down