Create forwarding rules service

This service encapsulates interactions with Cloud forwading rules
This removes copy pasted code between l4 ilb and l4 elb

pkg/forwardingrules/forwarding_rules.go

@@ -0,0 +1,62 @@
+package forwardingrules
+
+import (
+	"fmt"
+
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"k8s.io/ingress-gce/pkg/composite"
+	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/klog/v2"
+	"k8s.io/legacy-cloud-providers/gce"
+)
+
+type ForwardingRules struct {
+	cloud   *gce.Cloud
+	version meta.Version
+	scope   meta.KeyType
+}
+
+func NewForwardingRules(cloud *gce.Cloud, version meta.Version, scope meta.KeyType) *ForwardingRules {
+	return &ForwardingRules{
+		cloud:   cloud,
+		version: version,
+		scope:   scope,
+	}
+}
+
+func (frc *ForwardingRules) createKey(name string) (*meta.Key, error) {
+	return composite.CreateKey(frc.cloud, name, frc.scope)
+}
+
+func (frc *ForwardingRules) Create(forwardingRule *composite.ForwardingRule) error {
+	key, err := frc.createKey(forwardingRule.Name)
+	if err != nil {
+		klog.Errorf("Failed to create key for creating forwarding rule %s, err: %v", forwardingRule.Name, err)
+		return nil
+	}
+	return composite.CreateForwardingRule(frc.cloud, key, forwardingRule)
+}
+
+func (frc *ForwardingRules) Get(name string) (*composite.ForwardingRule, error) {
+	key, err := frc.createKey(name)
+	if err != nil {
+		return nil, fmt.Errorf("Failed to create key for fetching forwarding rule %s, err: %w", name, err)
+	}
+	fr, err := composite.GetForwardingRule(frc.cloud, key, frc.version)
+	if utils.IgnoreHTTPNotFound(err) != nil {
+		return nil, fmt.Errorf("Failed to get existing forwarding rule %s, err: %w", name, err)
+	}
+	return fr, nil
+}
+
+func (frc *ForwardingRules) Delete(name string) error {
+	key, err := frc.createKey(name)
+	if err != nil {
+		return fmt.Errorf("Failed to create key for deleting forwarding rule %s, err: %w", name, err)
+	}
+	err = composite.DeleteForwardingRule(frc.cloud, key, frc.version)
+	if utils.IgnoreHTTPNotFound(err) != nil {
+		return fmt.Errorf("Failed to delete forwarding rule %s, err: %w", name, err)
+	}
+	return nil
+}

pkg/forwardingrules/forwarding_rules_test.go

@@ -0,0 +1,218 @@
+package forwardingrules
+
+import (
+	"testing"
+
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
+	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"k8s.io/ingress-gce/pkg/composite"
+	"k8s.io/ingress-gce/pkg/utils"
+	"k8s.io/legacy-cloud-providers/gce"
+)
+
+func TestCreateForwardingRule(t *testing.T) {
+	testCases := []struct {
+		frRule *composite.ForwardingRule
+		desc   string
+	}{
+		{
+			frRule: &composite.ForwardingRule{
+				Name:                "elb",
+				Description:         "elb description",
+				LoadBalancingScheme: string(cloud.SchemeExternal),
+			},
+			desc: "Test creating external forwarding rule",
+		},
+		{
+			frRule: &composite.ForwardingRule{
+				Name:                "ilb",
+				Description:         "ilb description",
+				LoadBalancingScheme: string(cloud.SchemeInternal),
+			},
+			desc: "Test creating internal forwarding rule",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
+			frc := NewForwardingRules(fakeGCE, meta.VersionGA, meta.Regional)
+
+			err := frc.Create(tc.frRule)
+			if err != nil {
+				t.Fatalf("frc.Create(%v), returned error %v, want nil", tc.frRule, err)
+			}
+
+			verifyForwardingRuleExists(t, fakeGCE, tc.frRule.Name)
+		})
+	}
+}
+
+func TestGetForwardingRule(t *testing.T) {
+	elbForwardingRule := &composite.ForwardingRule{
+		Name:                "elb",
+		Version:             meta.VersionGA,
+		Scope:               meta.Regional,
+		LoadBalancingScheme: string(cloud.SchemeExternal),
+	}
+	ilbForwardingRule := &composite.ForwardingRule{
+		Name:                "ilb",
+		Version:             meta.VersionGA,
+		Scope:               meta.Regional,
+		LoadBalancingScheme: string(cloud.SchemeInternal),
+	}
+
+	testCases := []struct {
+		existingFwdRules []*composite.ForwardingRule
+		getFwdRuleName   string
+		expectedFwdRule  *composite.ForwardingRule
+		desc             string
+	}{
+		{
+			existingFwdRules: []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			getFwdRuleName:   elbForwardingRule.Name,
+			expectedFwdRule:  elbForwardingRule,
+			desc:             "Test getting external forwarding rule",
+		},
+		{
+			existingFwdRules: []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			getFwdRuleName:   ilbForwardingRule.Name,
+			expectedFwdRule:  ilbForwardingRule,
+			desc:             "Test getting internal forwarding rule",
+		},
+		{
+			existingFwdRules: []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			getFwdRuleName:   "non-existent-rule",
+			expectedFwdRule:  nil,
+			desc:             "Test getting non existent forwarding rule",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
+			for _, fr := range tc.existingFwdRules {
+				mustCreateForwardingRule(t, fakeGCE, fr)
+			}
+			frc := NewForwardingRules(fakeGCE, meta.VersionGA, meta.Regional)
+
+			fr, err := frc.Get(tc.getFwdRuleName)
+			if err != nil {
+				t.Fatalf("frc.Get(%v), returned error %v, want nil", tc.getFwdRuleName, err)
+			}
+
+			ignoreFields := cmpopts.IgnoreFields(composite.ForwardingRule{}, "SelfLink", "Region")
+			if !cmp.Equal(fr, tc.expectedFwdRule, ignoreFields) {
+				diff := cmp.Diff(fr, tc.expectedFwdRule, ignoreFields)
+				t.Errorf("frc.Get(s) returned %v, not equal to expectedFwdRule %v, diff: %v", fr, tc.expectedFwdRule, diff)
+			}
+		})
+	}
+}
+
+func TestDeleteForwardingRule(t *testing.T) {
+	elbForwardingRule := &composite.ForwardingRule{
+		Name:                "elb",
+		LoadBalancingScheme: string(cloud.SchemeExternal),
+	}
+	ilbForwardingRule := &composite.ForwardingRule{
+		Name:                "ilb",
+		LoadBalancingScheme: string(cloud.SchemeInternal),
+	}
+
+	testCases := []struct {
+		existingFwdRules    []*composite.ForwardingRule
+		deleteFwdRuleName   string
+		shouldExistFwdRules []*composite.ForwardingRule
+		desc                string
+	}{
+		{
+			existingFwdRules:    []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			deleteFwdRuleName:   elbForwardingRule.Name,
+			shouldExistFwdRules: []*composite.ForwardingRule{ilbForwardingRule},
+			desc:                "Delete elb forwarding rule",
+		},
+		{
+			existingFwdRules:    []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			deleteFwdRuleName:   ilbForwardingRule.Name,
+			shouldExistFwdRules: []*composite.ForwardingRule{elbForwardingRule},
+			desc:                "Delete ilb forwarding rule",
+		},
+		{
+			existingFwdRules:    []*composite.ForwardingRule{elbForwardingRule},
+			deleteFwdRuleName:   elbForwardingRule.Name,
+			shouldExistFwdRules: []*composite.ForwardingRule{},
+			desc:                "Delete single elb forwarding rule",
+		},
+		{
+			existingFwdRules:    []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			deleteFwdRuleName:   "non-existent",
+			shouldExistFwdRules: []*composite.ForwardingRule{elbForwardingRule, ilbForwardingRule},
+			desc:                "Delete non existent forwarding rule",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
+			for _, fr := range tc.existingFwdRules {
+				mustCreateForwardingRule(t, fakeGCE, fr)
+			}
+			frc := NewForwardingRules(fakeGCE, meta.VersionGA, meta.Regional)
+
+			err := frc.Delete(tc.deleteFwdRuleName)
+			if err != nil {
+				t.Fatalf("frc.Delete(%v), returned error %v, want nil", tc.deleteFwdRuleName, err)
+			}
+
+			verifyForwardingRuleNotExists(t, fakeGCE, tc.deleteFwdRuleName)
+			for _, fw := range tc.shouldExistFwdRules {
+				verifyForwardingRuleExists(t, fakeGCE, fw.Name)
+			}
+		})
+	}
+}
+
+func verifyForwardingRuleExists(t *testing.T, cloud *gce.Cloud, name string) {
+	t.Helper()
+	verifyForwardingRuleShouldExist(t, cloud, name, true)
+}
+
+func verifyForwardingRuleNotExists(t *testing.T, cloud *gce.Cloud, name string) {
+	t.Helper()
+	verifyForwardingRuleShouldExist(t, cloud, name, false)
+}
+
+func verifyForwardingRuleShouldExist(t *testing.T, cloud *gce.Cloud, name string, shouldExist bool) {
+	t.Helper()
+
+	key, err := composite.CreateKey(cloud, name, meta.Regional)
+	if err != nil {
+		t.Fatalf("Failed to create key for fetching forwarding rule %s, err: %v", name, err)
+	}
+	_, err = composite.GetForwardingRule(cloud, key, meta.VersionGA)
+	if err != nil {
+		if utils.IsNotFoundError(err) {
+			if shouldExist {
+				t.Errorf("Forwarding rule %s was not found, expected to exist", name)
+			}
+			return
+		}
+		t.Fatalf("composite.GetForwardingRule(_, %v, %v) returned error %v, want nil", key, meta.VersionGA, err)
+	}
+	if !shouldExist {
+		t.Errorf("Forwarding rule %s exists, expected to be not found", name)
+	}
+}
+
+func mustCreateForwardingRule(t *testing.T, cloud *gce.Cloud, fr *composite.ForwardingRule) {
+	t.Helper()
+
+	key := meta.RegionalKey(fr.Name, cloud.Region())
+	err := composite.CreateForwardingRule(cloud, key, fr)
+	if err != nil {
+		t.Fatalf("composite.CreateForwardingRule(_, %s, %v) returned error %v, want nil", key, fr, err)
+	}
+}

pkg/forwardingrules/interfaces.go

@@ -0,0 +1,12 @@
+package forwardingrules
+
+import (
+	"k8s.io/ingress-gce/pkg/composite"
+)
+
+// ForwardingRulesProvider is an interface to manage Google Cloud Forwarding Rules
+type ForwardingRulesProvider interface {
+	Get(name string) (*composite.ForwardingRule, error)
+	Create(forwardingRule *composite.ForwardingRule) error
+	Delete(name string) error
+}

pkg/l4lb/l4controller.go

@@ -178,7 +178,11 @@ func (l4c *L4Controller) shouldProcessService(service *v1.Service, l4 *loadbalan
 	frName := utils.LegacyForwardingRuleName(service)
 	// Processing should continue if an external forwarding rule exists. This can happen if the service is transitioning from External to Internal.
 	// The external forwarding rule might not be deleted by the time this controller starts processing the service.
-	if fr := l4.GetForwardingRule(frName, meta.VersionGA); fr != nil && fr.LoadBalancingScheme == string(cloud.SchemeInternal) {
+	fr, err := l4.GetForwardingRule(frName)
+	if err != nil {
+		klog.Errorf("Error getting l4 forwarding rule %s", frName)
+	}
+	if fr != nil && fr.LoadBalancingScheme == string(cloud.SchemeInternal) {
 		klog.Warningf("Ignoring update for service %s:%s as it contains legacy forwarding rule %q", service.Namespace, service.Name, frName)
 		return false
 	}

pkg/l4lb/l4netlbcontroller.go

@@ -276,7 +276,11 @@ func (lc *L4NetLBController) hasTargetPoolForwardingRule(service *v1.Service) bo
 		return false
 	}
 
-	existingFR := l4netlb.GetForwardingRule(frName, meta.VersionGA)
+	existingFR, err := l4netlb.GetForwardingRule(frName)
+	if err != nil {
+		klog.Errorf("Error getting forwarding rule %s", frName)
+		return false
+	}
 	if existingFR != nil && existingFR.Target != "" {
 		return true
 	}
@@ -323,7 +327,11 @@ func (lc *L4NetLBController) hasRBSForwardingRule(svc *v1.Service) bool {
 	if lc.hasForwardingRuleAnnotation(svc, frName) {
 		return true
 	}
-	existingFR := l4netlb.GetForwardingRule(frName, meta.VersionGA)
+	existingFR, err := l4netlb.GetForwardingRule(frName)
+	if err != nil {
+		klog.Errorf("Error getting forwarding rule %s", frName)
+		return false
+	}
 	return existingFR != nil && existingFR.LoadBalancingScheme == string(cloud.SchemeExternal) && existingFR.BackendService != ""
 }