Use GA API for ILB Global Access

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -28,7 +28,6 @@ import (
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
-	computebeta "google.golang.org/api/compute/v0.beta"
 	compute "google.golang.org/api/compute/v1"
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -169,24 +168,28 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v
 		return nil, err
 	}
 
-	newFRC := &forwardingRuleComposite{
+	fwdRuleDescription := &forwardingRuleDescription{ServiceName: nm.String()}
-		name:           loadBalancerName,
+	fwdRuleDescriptionString, err := fwdRuleDescription.marshal()
-		description:    &forwardingRuleDescription{ServiceName: nm.String()},
+	if err != nil {
-		ipAddress:      ipToUse,
+		return nil, err
-		backendService: backendServiceLink,
+	}
-		ports:          ports,
+	newFwdRule := &compute.ForwardingRule{
-		ipProtocol:     string(protocol),
+		Name:                loadBalancerName,
-		lbScheme:       string(scheme),
+		Description:         fwdRuleDescriptionString,
+		IPAddress:           ipToUse,
+		BackendService:      backendServiceLink,
+		Ports:               ports,
+		IPProtocol:          string(protocol),
+		LoadBalancingScheme: string(scheme),
 		// Given that CreateGCECloud will attempt to determine the subnet based off the network,
 		// the subnetwork should rarely be unknown.
-		subnetwork: subnetworkURL,
+		Subnetwork: subnetworkURL,
-		network:    g.networkURL,
+		Network:    g.networkURL,
 	}
 	if options.AllowGlobalAccess {
-		newFRC.allowGlobalAccess = options.AllowGlobalAccess
+		newFwdRule.AllowGlobalAccess = options.AllowGlobalAccess
-		newFRC.description.APIVersion = meta.VersionBeta
 	}
-	if err := g.ensureInternalForwardingRule(existingFwdRule, newFRC); err != nil {
+	if err := g.ensureInternalForwardingRule(existingFwdRule, newFwdRule); err != nil {
 		return nil, err
 	}
 
@@ -878,116 +881,6 @@ func getILBOptions(svc *v1.Service) ILBOptions {
 	}
 }
 
-// forwardingRuleComposite is a composite type encapsulating both the GA and Beta ForwardingRules.
-// It exposes methods to compute the ForwardingRule object based on the given parameters and to compare 2 composite types
-// based on the version string.
-type forwardingRuleComposite struct {
-	allowGlobalAccess bool
-	name              string
-	description       *forwardingRuleDescription
-	ipAddress         string
-	backendService    string
-	ports             []string
-	ipProtocol        string
-	lbScheme          string
-	subnetwork        string
-	network           string
-}
-
-func (f *forwardingRuleComposite) Version() meta.Version {
-	return f.description.APIVersion
-}
-
-func (f *forwardingRuleComposite) Equal(other *forwardingRuleComposite) bool {
-	return (f.ipAddress == "" || other.ipAddress == "" || f.ipAddress == other.ipAddress) &&
-		f.ipProtocol == other.ipProtocol &&
-		f.lbScheme == other.lbScheme &&
-		equalStringSets(f.ports, other.ports) &&
-		f.backendService == other.backendService &&
-		f.allowGlobalAccess == other.allowGlobalAccess &&
-		f.subnetwork == other.subnetwork
-}
-
-// toForwardingRuleComposite converts a compute beta or GA ForwardingRule into the composite type
-func toForwardingRuleComposite(rule interface{}) (frc *forwardingRuleComposite, err error) {
-	switch fr := rule.(type) {
-	case *compute.ForwardingRule:
-		frc = &forwardingRuleComposite{
-			name:           fr.Name,
-			ipAddress:      fr.IPAddress,
-			description:    &forwardingRuleDescription{APIVersion: meta.VersionGA},
-			backendService: fr.BackendService,
-			ports:          fr.Ports,
-			ipProtocol:     fr.IPProtocol,
-			lbScheme:       fr.LoadBalancingScheme,
-			subnetwork:     fr.Subnetwork,
-			network:        fr.Network,
-		}
-		if fr.Description != "" {
-			err = frc.description.unmarshal(fr.Description)
-		}
-		return frc, err
-	case *computebeta.ForwardingRule:
-		frc = &forwardingRuleComposite{
-			name:              fr.Name,
-			ipAddress:         fr.IPAddress,
-			description:       &forwardingRuleDescription{APIVersion: meta.VersionBeta},
-			backendService:    fr.BackendService,
-			ports:             fr.Ports,
-			ipProtocol:        fr.IPProtocol,
-			lbScheme:          fr.LoadBalancingScheme,
-			subnetwork:        fr.Subnetwork,
-			network:           fr.Network,
-			allowGlobalAccess: fr.AllowGlobalAccess,
-		}
-		if fr.Description != "" {
-			err = frc.description.unmarshal(fr.Description)
-		}
-		return frc, err
-	default:
-		return nil, fmt.Errorf("Invalid object type %T to compute ForwardingRuleComposite from", fr)
-	}
-}
-
-// ToBeta returns a Beta ForwardingRule from the composite type.
-func (f *forwardingRuleComposite) ToBeta() (*computebeta.ForwardingRule, error) {
-	descStr, err := f.description.marshal()
-	if err != nil {
-		return nil, fmt.Errorf("Failed to compute description for beta forwarding rule %s, err: %v", f.name, err)
-	}
-	return &computebeta.ForwardingRule{
-		Name:                f.name,
-		Description:         descStr,
-		IPAddress:           f.ipAddress,
-		BackendService:      f.backendService,
-		Ports:               f.ports,
-		IPProtocol:          f.ipProtocol,
-		LoadBalancingScheme: f.lbScheme,
-		Subnetwork:          f.subnetwork,
-		Network:             f.network,
-		AllowGlobalAccess:   f.allowGlobalAccess,
-	}, nil
-}
-
-// ToGA returns a GA ForwardingRule from the composite type.
-func (f *forwardingRuleComposite) ToGA() (*compute.ForwardingRule, error) {
-	descStr, err := f.description.marshal()
-	if err != nil {
-		return nil, fmt.Errorf("Failed to compute description for GA forwarding rule %s, err: %v", f.name, err)
-	}
-	return &compute.ForwardingRule{
-		Name:                f.name,
-		Description:         descStr,
-		IPAddress:           f.ipAddress,
-		BackendService:      f.backendService,
-		Ports:               f.ports,
-		IPProtocol:          f.ipProtocol,
-		LoadBalancingScheme: f.lbScheme,
-		Subnetwork:          f.subnetwork,
-		Network:             f.network,
-	}, nil
-}
-
 type forwardingRuleDescription struct {
 	ServiceName string       `json:"kubernetes.io/service-name"`
 	APIVersion  meta.Version `json:"kubernetes.io/api-version,omitempty"`
@@ -1021,32 +914,10 @@ func getFwdRuleAPIVersion(rule *compute.ForwardingRule) (meta.Version, error) {
 	return d.APIVersion, nil
 }
 
-func (g *Cloud) ensureInternalForwardingRule(existingFwdRule *compute.ForwardingRule, newFRC *forwardingRuleComposite) (err error) {
+func (g *Cloud) ensureInternalForwardingRule(existingFwdRule, newFwdRule *compute.ForwardingRule) (err error) {
 	if existingFwdRule != nil {
-		version, err := getFwdRuleAPIVersion(existingFwdRule)
+		if forwardingRulesEqual(existingFwdRule, newFwdRule) {
-		if err != nil {
+			klog.V(4).Infof("existingFwdRule == newFwdRule, no updates needed (existingFwdRule == %+v)", existingFwdRule)
-			return err
-		}
-		var oldFRC *forwardingRuleComposite
-		switch version {
-		case meta.VersionBeta:
-			var betaRule *computebeta.ForwardingRule
-			betaRule, err = g.GetBetaRegionForwardingRule(existingFwdRule.Name, g.region)
-			if err != nil {
-				return err
-			}
-			oldFRC, err = toForwardingRuleComposite(betaRule)
-		case meta.VersionGA:
-			oldFRC, err = toForwardingRuleComposite(existingFwdRule)
-		default:
-			klog.Errorf("invalid version string for %s, assuming GA", existingFwdRule.Name)
-			oldFRC, err = toForwardingRuleComposite(existingFwdRule)
-		}
-		if err != nil {
-			return err
-		}
-		if oldFRC.Equal(newFRC) {
-			klog.V(4).Infof("oldFRC == newFRC, no updates needed (oldFRC == %+v)", oldFRC)
 			return nil
 		}
 		klog.V(2).Infof("ensureInternalLoadBalancer(%v): deleting existing forwarding rule with IP address %v", existingFwdRule.Name, existingFwdRule.IPAddress)
@@ -1056,23 +927,20 @@ func (g *Cloud) ensureInternalForwardingRule(existingFwdRule *compute.Forwarding
 	}
 	// At this point, the existing rule has been deleted if required.
 	// Create the rule based on the api version determined
-	if newFRC.Version() == meta.VersionBeta {
+	klog.V(2).Infof("ensureInternalLoadBalancer(%v): creating forwarding rule", newFwdRule.Name)
-		klog.V(2).Infof("ensureInternalLoadBalancer(%v): creating beta forwarding rule", newFRC.name)
+	if err = g.CreateRegionForwardingRule(newFwdRule, g.region); err != nil {
-		var betaRule *computebeta.ForwardingRule
+		return err
-		betaRule, err = newFRC.ToBeta()
-		if err != nil {
-			return err
-		}
-		err = g.CreateBetaRegionForwardingRule(betaRule, g.region)
-	} else {
-		var gaRule *compute.ForwardingRule
-		klog.V(2).Infof("ensureInternalLoadBalancer(%v): creating ga forwarding rule", newFRC.name)
-		gaRule, err = newFRC.ToGA()
-		if err != nil {
-			return err
-		}
-		err = g.CreateRegionForwardingRule(gaRule, g.region)
 	}
-	klog.V(2).Infof("ensureInternalLoadBalancer(%v): created forwarding rule, err : %s", newFRC.name, err)
+	klog.V(2).Infof("ensureInternalLoadBalancer(%v): created forwarding rule", newFwdRule.Name)
-	return err
+	return nil
+}
+
+func forwardingRulesEqual(old, new *compute.ForwardingRule) bool {
+	return (old.IPAddress == "" || new.IPAddress == "" || old.IPAddress == new.IPAddress) &&
+		old.IPProtocol == new.IPProtocol &&
+		old.LoadBalancingScheme == new.LoadBalancingScheme &&
+		equalStringSets(old.Ports, new.Ports) &&
+		old.BackendService == new.BackendService &&
+		old.AllowGlobalAccess == new.AllowGlobalAccess &&
+		old.Subnetwork == new.Subnetwork
 }

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go

@@ -31,7 +31,6 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock"
-	computebeta "google.golang.org/api/compute/v0.beta"
 	"google.golang.org/api/compute/v1"
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -1135,17 +1134,13 @@ func TestEnsureInternalLoadBalancerGlobalAccess(t *testing.T) {
 		t.Errorf("Unexpected error %v", err)
 	}
 	assert.NotEmpty(t, status.Ingress)
-	betaRuleDescString := fmt.Sprintf(`{"kubernetes.io/service-name":"%s","kubernetes.io/api-version":"beta"}`, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}.String())
+	fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region)
-	fwdRule, err := gce.GetBetaRegionForwardingRule(lbName, gce.region)
+	if err != nil {
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
+	}
 	if !fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected false value for AllowGlobalAccess")
 	}
-	if fwdRule.Description != betaRuleDescString {
-		t.Errorf("Expected description %s, Got %s", betaRuleDescString, fwdRule.Description)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error %v", err)
-	}
 	// remove the annotation
 	delete(svc.Annotations, ServiceAnnotationILBAllowGlobalAccess)
 	status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes)
@@ -1153,17 +1148,13 @@ func TestEnsureInternalLoadBalancerGlobalAccess(t *testing.T) {
 		t.Errorf("Unexpected error %v", err)
 	}
 	assert.NotEmpty(t, status.Ingress)
-	gaRuleDescString := fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}.String())
+	fwdRule, err = gce.GetRegionForwardingRule(lbName, gce.region)
-	fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region)
 	if err != nil {
-		t.Errorf("Unexpected error %v", err)
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
 	}
 	if fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected true value for AllowGlobalAccess")
 	}
-	if fwdRule.Description != gaRuleDescString {
-		t.Errorf("Expected description %s, Got %s", gaRuleDescString, fwdRule.Description)
-	}
 	// Delete the service
 	err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc)
 	if err != nil {
@@ -1192,9 +1183,9 @@ func TestEnsureInternalLoadBalancerDisableGlobalAccess(t *testing.T) {
 		t.Errorf("Unexpected error %v", err)
 	}
 	assert.NotEmpty(t, status.Ingress)
-	fwdRule, err := gce.GetBetaRegionForwardingRule(lbName, gce.region)
+	fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region)
 	if err != nil {
-		t.Errorf("Unexpected error %v", err)
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
 	}
 	if !fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected false value for AllowGlobalAccess")
@@ -1207,9 +1198,9 @@ func TestEnsureInternalLoadBalancerDisableGlobalAccess(t *testing.T) {
 		t.Errorf("Unexpected error %v", err)
 	}
 	assert.NotEmpty(t, status.Ingress)
-	fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region)
+	fwdRule, err = gce.GetRegionForwardingRule(lbName, gce.region)
 	if err != nil {
-		t.Errorf("Unexpected error %v", err)
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
 	}
 	if fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected true value for AllowGlobalAccess")
@@ -1249,9 +1240,9 @@ func TestGlobalAccessChangeScheme(t *testing.T) {
 		t.Errorf("Unexpected error %v", err)
 	}
 	assert.NotEmpty(t, status.Ingress)
-	fwdRule, err := gce.GetBetaRegionForwardingRule(lbName, gce.region)
+	fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region)
 	if err != nil {
-		t.Errorf("Unexpected error %v", err)
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
 	}
 	if !fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected false value for AllowGlobalAccess")
@@ -1265,9 +1256,9 @@ func TestGlobalAccessChangeScheme(t *testing.T) {
 	assert.NotEmpty(t, status.Ingress)
 	// Firewall is deleted when the service is deleted
 	assertInternalLbResourcesDeleted(t, gce, svc, vals, false)
-	fwdRule, err = gce.GetBetaRegionForwardingRule(lbName, gce.region)
+	fwdRule, err = gce.GetRegionForwardingRule(lbName, gce.region)
 	if err != nil {
-		t.Errorf("Unexpected error %v", err)
+		t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err)
 	}
 	if fwdRule.AllowGlobalAccess {
 		t.Errorf("Unexpected true value for AllowGlobalAccess")
@@ -1309,77 +1300,78 @@ func TestUnmarshalEmptyAPIVersion(t *testing.T) {
 	}
 }
 
-func TestForwardingRuleCompositeEqual(t *testing.T) {
+func TestForwardingRulesEqual(t *testing.T) {
 	t.Parallel()
 
-	vals := DefaultTestClusterValues()
+	fwdRules := []*compute.ForwardingRule{
-	gce, err := fakeGCECloud(vals)
+		{
-	require.NoError(t, err)
+			Name:                "empty-ip-address-fwd-rule",
+			IPAddress:           "",
+			Ports:               []string{"123"},
+			IPProtocol:          "TCP",
+			LoadBalancingScheme: string(cloud.SchemeInternal),
+		},
+		{
+			Name:                "tcp-fwd-rule",
+			IPAddress:           "10.0.0.0",
+			Ports:               []string{"123"},
+			IPProtocol:          "TCP",
+			LoadBalancingScheme: string(cloud.SchemeInternal),
+		},
+		{
+			Name:                "udp-fwd-rule",
+			IPAddress:           "10.0.0.0",
+			Ports:               []string{"123"},
+			IPProtocol:          "UDP",
+			LoadBalancingScheme: string(cloud.SchemeInternal),
+		},
+		{
+			Name:                "global-access-fwd-rule",
+			IPAddress:           "10.0.0.0",
+			Ports:               []string{"123"},
+			IPProtocol:          "TCP",
+			LoadBalancingScheme: string(cloud.SchemeInternal),
+			AllowGlobalAccess:   true,
+		},
+	}
 
-	svc := fakeLoadbalancerService(string(LBTypeInternal))
+	for _, tc := range []struct {
-	lbName := gce.GetLoadBalancerName(context.TODO(), "", svc)
+		desc       string
-	gaRule := &compute.ForwardingRule{
+		oldFwdRule *compute.ForwardingRule
-		Name:                lbName,
+		newFwdRule *compute.ForwardingRule
-		IPAddress:           "",
+		expect     bool
-		Ports:               []string{"123"},
+	}{
-		IPProtocol:          "TCP",
+		{
-		LoadBalancingScheme: string(cloud.SchemeInternal),
+			desc:       "empty ip address matches any ip",
-	}
+			oldFwdRule: fwdRules[0],
-	betaRule := &computebeta.ForwardingRule{
+			newFwdRule: fwdRules[1],
-		Name:                lbName + "-beta",
+			expect:     true,
-		IPAddress:           "",
+		},
-		Description:         fmt.Sprintf(`{"kubernetes.io/service-name":"%s","apiVersion":"beta"}`, svc.Name),
+		{
-		Ports:               []string{"123"},
+			desc:       "global access enabled",
-		IPProtocol:          "TCP",
+			oldFwdRule: fwdRules[1],
-		LoadBalancingScheme: string(cloud.SchemeInternal),
+			newFwdRule: fwdRules[3],
-		AllowGlobalAccess:   false,
+			expect:     false,
-	}
+		},
-	betaRuleGlobalAccess := &computebeta.ForwardingRule{
+		{
-		Name:                lbName + "-globalaccess",
+			desc:       "IP protocol changed",
-		IPAddress:           "",
+			oldFwdRule: fwdRules[1],
-		Description:         fmt.Sprintf(`{"kubernetes.io/service-name":"%s","apiVersion":"beta"}`, svc.Name),
+			newFwdRule: fwdRules[2],
-		Ports:               []string{"123"},
+			expect:     false,
-		IPProtocol:          "TCP",
+		},
-		LoadBalancingScheme: string(cloud.SchemeInternal),
+		{
-		AllowGlobalAccess:   true,
+			desc:       "same forwarding rule",
-	}
+			oldFwdRule: fwdRules[3],
-	err = gce.CreateRegionForwardingRule(gaRule, gce.region)
+			newFwdRule: fwdRules[3],
-	if err != nil {
+			expect:     true,
-		t.Errorf("Unexpected error %v", err)
+		},
-	}
+	} {
-	err = gce.CreateBetaRegionForwardingRule(betaRule, gce.region)
+		t.Run(tc.desc, func(t *testing.T) {
-	if err != nil {
+			got := forwardingRulesEqual(tc.oldFwdRule, tc.newFwdRule)
-		t.Errorf("Unexpected error %v", err)
+			if got != tc.expect {
-	}
+				t.Errorf("forwardingRulesEqual(_, _) = %t, want %t", got, tc.expect)
-	err = gce.CreateBetaRegionForwardingRule(betaRuleGlobalAccess, gce.region)
+			}
-	if err != nil {
+		})
-		t.Errorf("Unexpected error %v", err)
-	}
-	frcGA, err := toForwardingRuleComposite(gaRule)
-	if err != nil {
-		t.Errorf("Unexpected error %v", err)
-	}
-	frcBeta, err := toForwardingRuleComposite(betaRule)
-	if err != nil {
-		t.Errorf("Unexpected error %v", err)
-	}
-	frcBetaGlobalAccess, err := toForwardingRuleComposite(betaRuleGlobalAccess)
-	if err != nil {
-		t.Errorf("Unexpected error %v", err)
-	}
-	if !frcGA.Equal(frcBeta) {
-		t.Errorf("Expected frcGA and frcBeta rules to be equal, got false")
-	}
-	if frcBeta.Equal(frcBetaGlobalAccess) {
-		t.Errorf("Expected FrcBeta and FrcBetaGlobalAccess rules to be unequal, got true")
-	}
-	if frcGA.Equal(frcBetaGlobalAccess) {
-		t.Errorf("Expected frcGA and frcBetaGlobalAccess rules to be unequal, got true")
-	}
-	// Enabling globalAccess in FrcBeta to make equality fail with FrcGA
-	frcBeta.allowGlobalAccess = true
-	if frcGA.Equal(frcBeta) {
-		t.Errorf("Expected frcGA and frcBeta rules to be unequal, got true")
 	}
 }