Skip to content

Fix edge case when cleaning up deny firewall rules on XPN without per… #3041

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

Open
TortillaZHawaii wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix edge case when cleaning up deny firewall rules on XPN without per… #3041

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
126 changes: 117 additions & 9 deletions pkg/l4resources/denytest/deny_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package denytest
import (
"context"
"errors"
"net/http"
"testing"
"time"

Expand All @@ -29,6 +30,7 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
)

const (
Expand Down Expand Up @@ -167,7 +169,7 @@ func TestDenyFirewall(t *testing.T) {
t.Run(start.desc+"_to_"+end.desc, func(t *testing.T) {
// Arrange
ctx := t.Context()
cloud, err := helperCloud(ctx)
cloud, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -247,7 +249,7 @@ func TestDenyIsNotCreatedWhenAllowPriorityUpdateFails(t *testing.T) {
// Arrange
ctx := t.Context()
svc := helperService([]v1.IPFamily{ipFamily})
gce, err := helperCloud(ctx)
gce, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -299,7 +301,7 @@ func TestDenyRespectsDisableNodeFirewallProvisioning(t *testing.T) {
// Arrange
ctx := t.Context()
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})
cloud, err := helperCloud(ctx)
cloud, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -352,7 +354,7 @@ func TestDenyRollforwardDoesNotBlockTraffic(t *testing.T) {
// We use dual stack as it's testing both IP stacks at the same time
ctx := t.Context()
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})
cloud, err := helperCloud(ctx)
cloud, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -445,7 +447,7 @@ func TestDenyRollback(t *testing.T) {
// Provision a LB with deny rules
ctx := t.Context()
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})
cloud, err := helperCloud(ctx)
cloud, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -593,7 +595,7 @@ func TestExportsCorrectDenyMetrics(t *testing.T) {
t.Parallel()
ctx := t.Context()
svc := helperService(tc.ipFamilies)
gce, err := helperCloud(ctx)
gce, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -627,7 +629,7 @@ func TestSkipRollbackCleanupIfDisabled(t *testing.T) {
ctx := t.Context()
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})

gce, err := helperCloud(ctx)
gce, err := helperCloud(ctx, gce.DefaultTestClusterValues())
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -670,6 +672,113 @@ func TestSkipRollbackCleanupIfDisabled(t *testing.T) {
}
}

// TestContinueOnXPN403s verifies that we don't error out on XPN clusters that don't have permissions to create firewalls
func TestContinueOnXPN403s(t *testing.T) {
testCases := []struct {
name string
denyFirewallEnabled bool
denyFirewallCleanupEnabled bool
}{
{
name: "disabled",
},
{
name: "rolled_back",
denyFirewallEnabled: true,
},
{
name: "enabled",
denyFirewallEnabled: true,
denyFirewallCleanupEnabled: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Arrange
ctx := t.Context()
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})
vals := gce.DefaultTestClusterValues()
vals.OnXPN = true
gce, err := helperCloud(ctx, vals)
if err != nil {
t.Fatal(err)
}

xpnErr := &googleapi.Error{
Code: http.StatusForbidden,
Message: "Required 'compute.firewalls.something' permission for 'projects/something/global/firewalls/something'.",
}

fwNames := []string{denyIPv4Name, denyIPv6Name, allowIPv4Name, allowIPv6Name}
forwardingRulesAnnotations := []string{
l4annotations.TCPForwardingRuleKey,
l4annotations.TCPForwardingRuleIPv6Key,
}

mockGCE := gce.Compute().(*cloud.MockGCE)
mockGCE.MockFirewalls.InsertHook = func(ctx context.Context, key *meta.Key, obj *compute.Firewall, m *cloud.MockFirewalls, options ...cloud.Option) (bool, error) {
return true, xpnErr
}
mockGCE.MockFirewalls.GetHook = func(ctx context.Context, key *meta.Key, m *cloud.MockFirewalls, options ...cloud.Option) (bool, *compute.Firewall, error) {
return false, nil, nil
}
mockGCE.MockFirewalls.DeleteHook = func(ctx context.Context, key *meta.Key, m *cloud.MockFirewalls, options ...cloud.Option) (bool, error) {
return true, xpnErr
}
mockGCE.MockFirewalls.PatchHook = func(ctx context.Context, key *meta.Key, obj *compute.Firewall, m *cloud.MockFirewalls, options ...cloud.Option) error {
return xpnErr
}
mockGCE.MockFirewalls.UpdateHook = mockGCE.MockFirewalls.PatchHook

log := klog.TODO()
ensurer := helperL4NetLB(gce, log, svc, true)
res := ensurer.EnsureFrontend([]string{nodeName}, svc, time.Now())
// Assert: we don't expect any errors to be returned
if res.Error != nil {
t.Fatal(res.Error)
}

// Assert: no firewalls were created
for _, name := range fwNames {
fw, _ := gce.GetFirewall(name)
if fw != nil {
t.Errorf("something is wrong with the test logic, the firewall %v should not have been created", name)
}
}

// Assert: forwarding rules exist either way
forwardingRuleNames := []string{}
for _, annotation := range forwardingRulesAnnotations {
frName := res.Annotations[annotation]
if frName == "" {
t.Errorf("something is wrong with the test logic, the forwarding rule %v should have been created", annotation)
}
forwardingRuleNames = append(forwardingRuleNames, frName)
_, err := gce.GetRegionForwardingRule(frName, gce.Region())
if err != nil {
t.Errorf("something is wrong with the test logic, the forwarding rule %v should have been created, but got %v", frName, err)
}
}

// Act: delete the service
res = ensurer.EnsureLoadBalancerDeleted(svc)
// Assert: we don't expect any errors to be returned
if res.Error != nil {
t.Fatal(res.Error)
}

// Assert: forwarding rules were cleaned up
for _, frName := range forwardingRuleNames {
fr, err := gce.GetRegionForwardingRule(frName, gce.Region())
if !utils.IsNotFoundError(err) || fr != nil {
t.Errorf("something is wrong with the test logic, the forwarding rule %v should have been cleaned up, but got err: %v and fw: %v", frName, err, fr)
}
}
})
}
}

func helperL4NetLB(cloud *gce.Cloud, log klog.Logger, svc *v1.Service, denyFirewall bool) *l4resources.L4NetLB {
return l4resources.NewL4NetLB(&l4resources.L4NetLBParams{
Service: svc,
Expand Down Expand Up @@ -724,8 +833,7 @@ func helperService(ipFamily []v1.IPFamily) *v1.Service {
}
}

func helperCloud(ctx context.Context) (*gce.Cloud, error) {
vals := gce.DefaultTestClusterValues()
func helperCloud(ctx context.Context, vals gce.TestClusterValues) (*gce.Cloud, error) {
gce := gce.NewFakeGCECloud(vals)

firewallTracker := &firewallTracker{}
Expand Down
22 changes: 13 additions & 9 deletions pkg/l4resources/l4netlb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/cache"
"k8s.io/client-go/tools/record"
"k8s.io/cloud-provider-gcp/providers/gce"
"k8s.io/cloud-provider/service/helpers"
"k8s.io/ingress-gce/pkg/address"
"k8s.io/ingress-gce/pkg/backends"
Expand All @@ -43,6 +42,8 @@ import (
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/namer"
"k8s.io/klog/v2"

"k8s.io/cloud-provider-gcp/providers/gce"
)

const (
Expand Down Expand Up @@ -102,7 +103,6 @@ type L4NetLBSyncResult struct {
}

func NewL4SyncResult(syncType string, startTime time.Time, svc *corev1.Service, isMultinet bool, enabledStrongSessionAffinity bool, isWeightedLBPodsPerNode bool, useNEGs bool) *L4NetLBSyncResult {

backendType := metrics.L4BackendTypeInstanceGroup
if useNEGs || isMultinet {
backendType = metrics.L4BackendTypeNEG
Expand Down Expand Up @@ -651,32 +651,36 @@ func (l4netlb *L4NetLB) softlyCleanUpDenyFirewallsWhenRolledBack() error {
// if we don't use deny flag, make sure that a leftover deny rule is cleaned up
// cleanUpDenyFirewallRule does checks if the rule exists before attempting to delete
// so it won't leave 404 errors in audit logs
if err := cleanUpDenyFirewallRule(l4netlb.cloud, l4netlb.namer.L4FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), logger); err != nil {
if err := l4netlb.cleanUpDenyFirewallRule(l4netlb.namer.L4FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), logger); err != nil {
return err
}
if err := cleanUpDenyFirewallRule(l4netlb.cloud, l4netlb.namer.L4IPv6FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), logger); err != nil {
if err := l4netlb.cleanUpDenyFirewallRule(l4netlb.namer.L4IPv6FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), logger); err != nil {
return err
}
return nil
}

// cleanUpDenyFirewallRule will attempt the deletion only after checking that it exists with GET call.
// This is done to avoid polluting Audit Logs with 404 errors (even though the 404 might be expected).
func cleanUpDenyFirewallRule(cloud *gce.Cloud, fwName string, logger klog.Logger) error {
func (l4netlb *L4NetLB) cleanUpDenyFirewallRule(fwName string, logger klog.Logger) error {
log := logger.WithName("cleanUpDenyFirewallRule").WithValues("firewallName", fwName)

// Skip deleting if it doesn't exist to not produce noisy audit logs
fa := firewalls.NewFirewallAdapter(cloud)
fa := firewalls.NewFirewallAdapter(l4netlb.cloud)
if _, err := fa.GetFirewall(fwName); err != nil {
if utils.IsNotFoundError(err) {
log.V(3).Info("no deny firewall found to delete", "firewallName", fwName)
return nil // no need for cleanup
}
if utils.IsForbiddenError(err) && l4netlb.cloud.OnXPN() {
log.V(3).Info("can't delete firewall without permissions for XPN, skipping", "firewallName", fwName)
return nil // no need for cleanup
}
return err
}

log.V(3).Info("deny firewall exists, deleting it", "firewallName", fwName)
return firewalls.EnsureL4FirewallRuleDeleted(cloud, fwName, log)
return l4netlb.deleteFirewall(fwName, logger)
}

// EnsureLoadBalancerDeleted performs a cleanup of all GCE resources for the given loadbalancer service.
Expand Down Expand Up @@ -760,8 +764,8 @@ func (l4netlb *L4NetLB) deleteIPv4ResourcesAnnotationBased(result *L4NetLBSyncRe
result.Error = err
}
}
if shouldIgnoreAnnotations || l4netlb.hasAnnotation(l4annotations.FirewallRuleDenyKey) {
err = cleanUpDenyFirewallRule(l4netlb.cloud, l4netlb.namer.L4FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), l4netlb.svcLogger)
if l4netlb.enableDenyFirewallsRollbackCleanup && (shouldIgnoreAnnotations || l4netlb.hasAnnotation(l4annotations.FirewallRuleDenyKey)) {
err = l4netlb.cleanUpDenyFirewallRule(l4netlb.namer.L4FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), l4netlb.svcLogger)
if err != nil {
l4netlb.svcLogger.Error(err, "Failed to delete deny firewall rule for NetLB RBS service")
result.GCEResourceInError = l4annotations.FirewallDenyRuleResource
Expand Down
16 changes: 10 additions & 6 deletions pkg/l4resources/l4netlbipv6.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ func (l4netlb *L4NetLB) deleteIPv6ResourcesAnnotationBased(syncResult *L4NetLBSy
if shouldIgnoreAnnotations || l4netlb.hasAnnotation(l4annotations.FirewallRuleIPv6Key) {
l4netlb.deleteIPv6NodesFirewall(syncResult)
}

if l4netlb.enableDenyFirewallsRollbackCleanup && (shouldIgnoreAnnotations || l4netlb.hasAnnotation(l4annotations.FirewallRuleDenyIPv6Key)) {
denyName := l4netlb.namer.L4IPv6FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name)
err := l4netlb.cleanUpDenyFirewallRule(denyName, l4netlb.svcLogger)
if err != nil {
l4netlb.svcLogger.Error(err, "Failed to delete deny firewall rule for NetLB RBS service", "fwDenyName", denyName)
syncResult.GCEResourceInError = l4annotations.FirewallDenyRuleResource
syncResult.Error = err
}
}
}

func (l4netlb *L4NetLB) ipv6FRName() string {
Expand Down Expand Up @@ -216,12 +226,6 @@ func (l4netlb *L4NetLB) deleteIPv6NodesFirewall(syncResult *L4NetLBSyncResult) {
syncResult.GCEResourceInError = l4annotations.FirewallRuleIPv6Resource
syncResult.Error = err
}

if err := cleanUpDenyFirewallRule(l4netlb.cloud, l4netlb.namer.L4IPv6FirewallDeny(l4netlb.Service.Namespace, l4netlb.Service.Name), fwLogger); err != nil {
fwLogger.Error(err, "Failed to delete ipv6 deny firewall rule for external loadbalancer service")
syncResult.GCEResourceInError = l4annotations.FirewallDenyRuleIPv6Resource
syncResult.Error = err
}
}

func (l4netlb *L4NetLB) ipv6SubnetURL() (string, error) {
Expand Down