Merge pull request #1698 from kl52752/user-error-metric

Do not include User Errors in L4NetLBInError metric

pkg/l4lb/l4netlbcontroller_test.go

@@ -19,7 +19,6 @@ package l4lb
 import (
 	"context"
 	"fmt"
-	"google.golang.org/api/googleapi"
 	"math/rand"
 	"net/http"
 	"reflect"
@@ -29,6 +28,8 @@ import (
 	"testing"
 	"time"
 
+	"google.golang.org/api/googleapi"
+
 	"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"
@@ -604,7 +605,7 @@ func TestProcessServiceCreationFailed(t *testing.T) {
 		key, _ := common.KeyFunc(svc)
 		err := lc.sync(key)
 		if err == nil || err.Error() != param.expectedError {
-			t.Errorf("Error mismatch '%v' != '%v'", err.Error(), param.expectedError)
+			t.Errorf("Error mismatch '%v' != '%v'", err, param.expectedError)
 		}
 	}
 }
@@ -876,10 +877,6 @@ func updateAndAssertExternalTrafficPolicy(newSvc *v1.Service, lc *L4NetLBControl
 	return nil
 }
 
-func isWrongNetworkTierError(err error) bool {
-	return err != nil && strings.Contains(err.Error(), "does not have the expected network tier")
-}
-
 func TestControllerUserIPWithStandardNetworkTier(t *testing.T) {
 	// Network Tier from User Static Address should match network tier from forwarding rule.
 	// Premium Network Tier is default for creating forwarding rule so if User wants to use Standard Network Tier for Static Address
@@ -893,7 +890,7 @@ func TestControllerUserIPWithStandardNetworkTier(t *testing.T) {
 	key, _ := common.KeyFunc(svc)
 	addUsersStaticAddress(lc, cloud.NetworkTierStandard)
 	// Sync should return error that Network Tier mismatch because we cannot tear User Managed Address.
-	if err := lc.sync(key); !isWrongNetworkTierError(err) {
+	if err := lc.sync(key); !utils.IsNetworkTierError(err) {
 		t.Errorf("Expected error when trying to ensure service with wrong Network Tier, err: %v", err)
 	}
 	svc.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierStandard)

pkg/loadbalancers/address_manager.go

@@ -165,6 +165,19 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err
 		return addr.Address, IPAddrManaged, nil
 	}
 
+	if utils.IsNetworkTierMismatchGCEError(reserveErr) {
+		receivedNetworkTier := cloud.NetworkTierPremium
+		if receivedNetworkTier == am.networkTier {
+			// We don't have information of current ephemeral IP address Network Tier since
+			// we try to reserve the address so we need to check against desired Network Tier and set the opposite one.
+			// This is just for error message.
+			receivedNetworkTier = cloud.NetworkTierStandard
+		}
+		resource := fmt.Sprintf("Reserved static IP (%v)", am.name)
+		networkTierError := utils.NewNetworkTierErr(resource, string(am.networkTier), string(receivedNetworkTier))
+		return "", IPAddrUndefined, networkTierError
+	}
+
 	if !utils.IsHTTPErrorCode(reserveErr, http.StatusConflict) && !utils.IsHTTPErrorCode(reserveErr, http.StatusBadRequest) {
 		// If the IP is already reserved:
 		//    by an internal address: a StatusConflict is returned
@@ -187,7 +200,7 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err
 
 	// Check that the address attributes are as required.
 	if err := am.validateAddress(addr); err != nil {
-		return "", IPAddrUndefined, err
+		return "", IPAddrUndefined, fmt.Errorf("address (%q) validation failed, err: %w", addr.Name, err)
 	}
 
 	if am.isManagedAddress(addr) {
@@ -205,13 +218,13 @@ func (am *addressManager) ensureAddressReservation() (string, IPAddressType, err
 
 func (am *addressManager) validateAddress(addr *compute.Address) error {
 	if am.targetIP != "" && am.targetIP != addr.Address {
-		return fmt.Errorf("address %q does not have the expected IP %q, actual: %q", addr.Name, am.targetIP, addr.Address)
+		return fmt.Errorf("IP mismatch, expected %q, actual: %q", am.targetIP, addr.Address)
 	}
 	if addr.AddressType != string(am.addressType) {
-		return fmt.Errorf("address %q does not have the expected address type %q, actual: %q", addr.Name, am.addressType, addr.AddressType)
+		return fmt.Errorf("address type mismatch, expected %q, actual: %q", am.addressType, addr.AddressType)
 	}
 	if addr.NetworkTier != am.networkTier.ToGCEValue() {
-		return fmt.Errorf("address %q does not have the expected network tier %q, actual: %q", addr.Name, am.networkTier.ToGCEValue(), addr.NetworkTier)
+		return utils.NewNetworkTierErr(fmt.Sprintf("Static IP (%v)", am.name), am.networkTier.ToGCEValue(), addr.NetworkTier)
 	}
 	return nil
 }
@@ -238,7 +251,7 @@ func (am *addressManager) TearDownAddressIPIfNetworkTierMismatch() error {
 	}
 	if addr != nil && addr.NetworkTier != am.networkTier.ToGCEValue() {
 		if !am.isManagedAddress(addr) {
-			return fmt.Errorf("User specific address IP (%v) network tier mismatch %v != %v ", am.targetIP, addr.NetworkTier, am.networkTier)
+			return utils.NewNetworkTierErr(fmt.Sprintf("User specific address IP (%v)", am.name), string(am.networkTier), addr.NetworkTier)
 		}
 		klog.V(3).Infof("Deleting IP address %v because has wrong network tier", am.targetIP)
 		am.svc.DeleteRegionAddress(addr.Name, am.targetIP)

pkg/loadbalancers/address_manager_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock"
+	"k8s.io/ingress-gce/pkg/test"
 	"k8s.io/ingress-gce/pkg/utils"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
@@ -148,8 +149,11 @@ func TestAddressManagerExternallyOwnedWrongNetworkTier(t *testing.T) {
 	err = svc.ReserveRegionAddress(addr, vals.Region)
 	require.NoError(t, err, "")
 	mgr := newAddressManager(svc, testSvcName, vals.Region, testSubnet, testLBName, targetIP, cloud.SchemeInternal, cloud.NetworkTierPremium)
+	svc.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = test.InsertAddressNetworkErrorHook
 	_, _, err = mgr.HoldAddress()
-	require.Error(t, err, "does not have the expected network tier")
+	if err == nil || !utils.IsNetworkTierError(err) {
+		t.Fatalf("mgr.HoldAddress() = %v, utils.IsNetworkTierError(err) = %t, want %t", err, utils.IsNetworkTierError(err), true)
+	}
 }
 
 // TestAddressManagerExternallyOwned tests the case where the address exists but isn't

pkg/loadbalancers/forwarding_rules.go

@@ -388,6 +388,11 @@ func (l4netlb *L4NetLB) ensureExternalForwardingRule(bsLink string) (*composite.
 	}
 
 	if existingFwdRule != nil {
+		if existingFwdRule.NetworkTier != fr.NetworkTier {
+			resource := fmt.Sprintf("Forwarding rule (%v)", frName)
+			networkTierMismatchError := utils.NewNetworkTierErr(resource, existingFwdRule.NetworkTier, fr.NetworkTier)
+			return nil, IPAddrUndefined, networkTierMismatchError
+		}
 		equal, err := Equal(existingFwdRule, fr)
 		if err != nil {
 			return existingFwdRule, IPAddrUndefined, err

pkg/loadbalancers/l4netlb.go

@@ -142,6 +142,8 @@ func (l4netlb *L4NetLB) EnsureFrontend(nodeNames []string, svc *corev1.Service)
 	result.Annotations[annotations.BackendServiceKey] = name
 	fr, ipAddrType, err := l4netlb.ensureExternalForwardingRule(bs.SelfLink)
 	if err != nil {
+		// User can misconfigure the forwarding rule if Network Tier will not match service level Network Tier.
+		result.MetricsState.IsUserError = utils.IsUserError(err)
 		result.GCEResourceInError = annotations.ForwardingRuleResource
 		result.Error = fmt.Errorf("Failed to ensure forwarding rule - %w", err)
 		return result

pkg/loadbalancers/l4netlb_test.go

@@ -38,12 +38,9 @@ import (
 )
 
 const (
-	managedIP    = true
-	unmanagedIP  = false
-	premiumTier  = true
-	standardTier = false
-	usersIP      = "35.10.211.60"
-	userAddrName = "UserStaticAddress"
+	usersIP        = "35.10.211.60"
+	usersIPPremium = "35.10.211.70"
+	userAddrName   = "UserStaticAddress"
 )
 
 func TestEnsureL4NetLoadBalancer(t *testing.T) {
@@ -71,7 +68,7 @@ func TestEnsureL4NetLoadBalancer(t *testing.T) {
 		t.Errorf("Annotations error: %v", err)
 	}
 	assertNetLbResources(t, svc, l4netlb, nodeNames)
-	if err := checkMetrics(result.MetricsState, managedIP, premiumTier); err != nil {
+	if err := checkMetrics(result.MetricsState /*isManaged = */, true /*isPremium = */, true /*isUserError =*/, false); err != nil {
 		t.Errorf("Metrics error: %v", err)
 	}
 }
@@ -279,11 +276,12 @@ func assertNetLbResources(t *testing.T, apiService *v1.Service, l4NetLb *L4NetLB
 	}
 }
 
-func TestMetricsForUserStaticService(t *testing.T) {
+func TestMetricsForStandardNetworkTier(t *testing.T) {
 	nodeNames := []string{"test-node-1"}
 	vals := gce.DefaultTestClusterValues()
 	fakeGCE := getFakeGCECloud(vals)
-	createUserStaticIP(fakeGCE, vals.Region)
+	createUserStaticIPInStandardTier(fakeGCE, vals.Region)
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.GetHook = test.GetRBSForwardingRuleInStandardTier
 
 	svc := test.NewL4NetLBRBSService(8080)
 	svc.Spec.LoadBalancerIP = usersIP
@@ -299,12 +297,37 @@ func TestMetricsForUserStaticService(t *testing.T) {
 	if result.Error != nil {
 		t.Errorf("Failed to ensure loadBalancer, err %v", result.Error)
 	}
-	if err := checkMetrics(result.MetricsState, unmanagedIP, standardTier); err != nil {
+	if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, false); err != nil {
+		t.Errorf("Metrics error: %v", err)
+	}
+	// Check that service sync will return error if User Address IP Network Tier mismatch with service Network Tier.
+	svc.ObjectMeta.Annotations[annotations.NetworkTierAnnotationKey] = string(cloud.NetworkTierPremium)
+	result = l4netlb.EnsureFrontend(nodeNames, svc)
+	if result.Error == nil || !utils.IsNetworkTierError(result.Error) {
+		t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error)
+	}
+	if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil {
+		t.Errorf("Metrics error: %v", err)
+	}
+	// Check that when network tier annotation will be deleted which will change desired service Network Tier to PREMIUM
+	// service sync will return User Error because we do not support updating forwarding rule.
+	// Forwarding rule with wrong tier should be tear down and it can be done only via annotation change.
+
+	// Crete new Static IP in Premium Network Tier to match default service Network Tier.
+	createUserStaticIPInPremiumTier(fakeGCE, vals.Region)
+	svc.Spec.LoadBalancerIP = usersIPPremium
+	delete(svc.ObjectMeta.Annotations, annotations.NetworkTierAnnotationKey)
+
+	result = l4netlb.EnsureFrontend(nodeNames, svc)
+	if result.Error == nil || !utils.IsNetworkTierError(result.Error) {
+		t.Errorf("LoadBalancer sync should return Network Tier error, err %v", result.Error)
+	}
+	if err := checkMetrics(result.MetricsState /*isManaged = */, false /*isPremium = */, false /*isUserError =*/, true); err != nil {
 		t.Errorf("Metrics error: %v", err)
 	}
 }
 
-func createUserStaticIP(fakeGCE *gce.Cloud, region string) {
+func createUserStaticIPInStandardTier(fakeGCE *gce.Cloud, region string) {
 	fakeGCE.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = mock.InsertAddressHook
 	fakeGCE.Compute().(*cloud.MockGCE).MockAlphaAddresses.X = mock.AddressAttributes{}
 	fakeGCE.Compute().(*cloud.MockGCE).MockAddresses.X = mock.AddressAttributes{}
@@ -317,13 +340,29 @@ func createUserStaticIP(fakeGCE *gce.Cloud, region string) {
 	}
 	fakeGCE.ReserveRegionAddress(newAddr, region)
 }
+func createUserStaticIPInPremiumTier(fakeGCE *gce.Cloud, region string) {
+	fakeGCE.Compute().(*cloud.MockGCE).MockAddresses.InsertHook = mock.InsertAddressHook
+	fakeGCE.Compute().(*cloud.MockGCE).MockAlphaAddresses.X = mock.AddressAttributes{}
+	fakeGCE.Compute().(*cloud.MockGCE).MockAddresses.X = mock.AddressAttributes{}
+	newAddr := &ga.Address{
+		Name:        "userAddrNamePremium",
+		Description: fmt.Sprintf(`{"kubernetes.io/service-name":"%s"}`, "userAddrName"),
+		Address:     usersIPPremium,
+		AddressType: string(cloud.SchemeExternal),
+		NetworkTier: cloud.NetworkTierPremium.ToGCEValue(),
+	}
+	fakeGCE.ReserveRegionAddress(newAddr, region)
+}
 
-func checkMetrics(m metrics.L4NetLBServiceState, isManaged, isPremium bool) error {
+func checkMetrics(m metrics.L4NetLBServiceState, isManaged, isPremium, isUserError bool) error {
 	if m.IsPremiumTier != isPremium {
 		return fmt.Errorf("L4 NetLB metric premium tier should be %v", isPremium)
 	}
 	if m.IsManagedIP != isManaged {
 		return fmt.Errorf("L4 NetLB metric is managed ip should be %v", isManaged)
 	}
+	if m.IsUserError != isUserError {
+		return fmt.Errorf("L4 NetLB metric is user error should be %v", isUserError)
+	}
 	return nil
 }

pkg/metrics/features.go

@@ -124,6 +124,8 @@ const (
 	l4NetLBInSuccess = feature("L4NetLBInSuccess")
 	// l4NetLBInInError feature specifies that an error had occurred while creating/updating GCE Load Balancer.
 	l4NetLBInError = feature("L4NetLBInError")
+	// l4NetLBInInUserError feature specifies that an error cause by User misconfiguration had occurred while creating/updating GCE Load Balancer.
+	l4NetLBInUserError = feature("L4NetLBInUserError")
 
 	// PSC Features
 	sa          = feature("ServiceAttachments")