diff --git a/pkg/l4resources/denytest/deny_test.go b/pkg/l4resources/denytest/deny_test.go index 82e354bd91..f5229b3b11 100644 --- a/pkg/l4resources/denytest/deny_test.go +++ b/pkg/l4resources/denytest/deny_test.go @@ -4,6 +4,7 @@ package denytest import ( "context" "errors" + "net/http" "testing" "time" @@ -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 ( @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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) } @@ -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, @@ -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{} diff --git a/pkg/l4resources/l4netlb.go b/pkg/l4resources/l4netlb.go index 1511382dec..bb0d681151 100644 --- a/pkg/l4resources/l4netlb.go +++ b/pkg/l4resources/l4netlb.go @@ -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" @@ -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 ( @@ -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 @@ -651,10 +651,10 @@ 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 @@ -662,21 +662,25 @@ func (l4netlb *L4NetLB) softlyCleanUpDenyFirewallsWhenRolledBack() error { // 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. @@ -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 diff --git a/pkg/l4resources/l4netlbipv6.go b/pkg/l4resources/l4netlbipv6.go index b77402d731..85548e2579 100644 --- a/pkg/l4resources/l4netlbipv6.go +++ b/pkg/l4resources/l4netlbipv6.go @@ -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 { @@ -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) {