Add UserError Status to ILB DualStack metrics

pkg/l4lb/l4controller.go

@@ -37,6 +37,7 @@ import (
 	"k8s.io/ingress-gce/pkg/forwardingrules"
 	l4metrics "k8s.io/ingress-gce/pkg/l4lb/metrics"
 	"k8s.io/ingress-gce/pkg/loadbalancers"
+	"k8s.io/ingress-gce/pkg/metrics"
 	negtypes "k8s.io/ingress-gce/pkg/neg/types"
 	"k8s.io/ingress-gce/pkg/utils"
 	"k8s.io/ingress-gce/pkg/utils/common"
@@ -233,6 +234,9 @@ func (l4c *L4Controller) processServiceCreateOrUpdate(service *v1.Service) *load
 			"Error syncing load balancer: %v", syncResult.Error)
 		if utils.IsUserError(syncResult.Error) {
 			syncResult.MetricsState.IsUserError = true
+			if l4c.enableDualStack {
+				syncResult.DualStackMetricsState.Status = metrics.StatusUserError
+			}
 		}
 		return syncResult
 	}
@@ -502,7 +506,7 @@ func (l4c *L4Controller) publishMetrics(result *loadbalancers.L4ILBSyncResult, n
 			klog.V(6).Infof("Internal L4 Loadbalancer for Service %s deleted, removing its state from metrics cache", namespacedName)
 			l4c.ctx.ControllerMetrics.DeleteL4ILBService(namespacedName)
 			if l4c.enableDualStack {
-				klog.V(6).Infof("Internal L4 Loadbalancer for Service %s deleted, removing its state from metrics cache", namespacedName)
+				klog.V(6).Infof("Internal L4 DualStack LoadBalancer for Service %s deleted, removing its state from metrics cache", namespacedName)
 				l4c.ctx.ControllerMetrics.DeleteL4ILBDualStackService(namespacedName)
 			}
 		}

pkg/l4lb/l4controller_test.go

@@ -25,6 +25,7 @@ import (
 
 	"google.golang.org/api/googleapi"
 	"k8s.io/ingress-gce/pkg/loadbalancers"
+	"k8s.io/ingress-gce/pkg/metrics"
 	"k8s.io/ingress-gce/pkg/utils"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
@@ -67,161 +68,6 @@ var (
 	}
 )
 
-func newServiceController(t *testing.T, fakeGCE *gce.Cloud) *L4Controller {
-	kubeClient := fake.NewSimpleClientset()
-
-	vals := gce.DefaultTestClusterValues()
-	namer := namer.NewNamer(clusterUID, "")
-
-	stopCh := make(chan struct{})
-	ctxConfig := context.ControllerContextConfig{
-		Namespace:    api_v1.NamespaceAll,
-		ResyncPeriod: 1 * time.Minute,
-		NumL4Workers: 5,
-	}
-	ctx := context.NewControllerContext(nil, kubeClient, nil, nil, nil, nil, nil, fakeGCE, namer, "" /*kubeSystemUID*/, ctxConfig)
-	// Add some nodes so that NEG linker kicks in during ILB creation.
-	nodes, err := test.CreateAndInsertNodes(ctx.Cloud, []string{"instance-1"}, vals.ZoneName)
-	if err != nil {
-		t.Errorf("Failed to add new nodes, err  %v", err)
-	}
-	for _, n := range nodes {
-		ctx.NodeInformer.GetIndexer().Add(n)
-	}
-	return NewILBController(ctx, stopCh)
-}
-
-func newFakeGCE() *gce.Cloud {
-	vals := gce.DefaultTestClusterValues()
-	fakeGCE := gce.NewFakeGCECloud(vals)
-	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = loadbalancers.InsertForwardingRuleHook
-	return fakeGCE
-}
-
-func newFakeGCEWithInsertError() *gce.Cloud {
-	vals := gce.DefaultTestClusterValues()
-	fakeGCE := gce.NewFakeGCECloud(vals)
-	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = mock.InsertForwardingRulesInternalErrHook
-	return fakeGCE
-}
-
-func newFakeGCEWithUserInsertError() *gce.Cloud {
-	vals := gce.DefaultTestClusterValues()
-	fakeGCE := gce.NewFakeGCECloud(vals)
-	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = test.InsertForwardingRuleErrorHook(&googleapi.Error{Code: http.StatusConflict, Message: "IP_IN_USE_BY_ANOTHER_RESOURCE - IP '1.1.1.1' is already being used by another resource."})
-	return fakeGCE
-}
-
-func addILBService(l4c *L4Controller, svc *api_v1.Service) {
-	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Create(context2.TODO(), svc, v1.CreateOptions{})
-	l4c.ctx.ServiceInformer.GetIndexer().Add(svc)
-}
-
-func updateILBService(l4c *L4Controller, svc *api_v1.Service) {
-	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Update(context2.TODO(), svc, v1.UpdateOptions{})
-	l4c.ctx.ServiceInformer.GetIndexer().Update(svc)
-}
-
-func deleteILBService(l4c *L4Controller, svc *api_v1.Service) {
-	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Delete(context2.TODO(), svc.Name, v1.DeleteOptions{})
-	l4c.ctx.ServiceInformer.GetIndexer().Delete(svc)
-}
-
-func addNEG(l4c *L4Controller, svc *api_v1.Service) {
-	// Also create a fake NEG for this service since the sync code will try to link the backend service to NEG
-	negName := l4c.namer.L4Backend(svc.Namespace, svc.Name)
-	neg := &composite.NetworkEndpointGroup{Name: negName}
-	key := meta.ZonalKey(negName, testGCEZone)
-	composite.CreateNetworkEndpointGroup(l4c.ctx.Cloud, key, neg)
-}
-
-func getKeyForSvc(svc *api_v1.Service, t *testing.T) string {
-	key, err := common.KeyFunc(svc)
-	if err != nil {
-		t.Fatalf("Failed to get key for service %v, err : %v", svc, err)
-	}
-	return key
-}
-
-func calculateExpectedAnnotationsKeys(svc *api_v1.Service) []string {
-	expectedAnnotations := ilbCommonAnnotationKeys
-	if utils.NeedsIPv4(svc) {
-		expectedAnnotations = append(expectedAnnotations, ilbIPv4AnnotationKeys...)
-	}
-	if utils.NeedsIPv6(svc) {
-		expectedAnnotations = append(expectedAnnotations, ilbIPv6AnnotationKeys...)
-	}
-	return expectedAnnotations
-}
-
-func verifyILBServiceProvisioned(t *testing.T, svc *api_v1.Service) {
-	t.Helper()
-
-	if !common.HasGivenFinalizer(svc.ObjectMeta, common.ILBFinalizerV2) {
-		t.Errorf("ILB v2 finalizer is not found, expected to exist, svc %+v", svc)
-	}
-	if len(svc.Status.LoadBalancer.Ingress) == 0 || svc.Status.LoadBalancer.Ingress[0].IP == "" {
-		t.Errorf("Invalid LoadBalancer status field in service - %+v", svc.Status.LoadBalancer)
-	}
-
-	expectedAnnotationsKeys := calculateExpectedAnnotationsKeys(svc)
-	var missingKeys []string
-	for _, key := range expectedAnnotationsKeys {
-		if _, ok := svc.Annotations[key]; !ok {
-			missingKeys = append(missingKeys, key)
-		}
-	}
-	if len(missingKeys) > 0 {
-		t.Errorf("Cannot find annotations %v in ILB service, Got %v", missingKeys, svc.Annotations)
-	}
-}
-
-func verifyILBServiceNotProvisioned(t *testing.T, svc *api_v1.Service) {
-	t.Helper()
-
-	if common.HasGivenFinalizer(svc.ObjectMeta, common.ILBFinalizerV2) {
-		t.Errorf("ILB v2 finalizer should not exist on service %+v", svc)
-	}
-
-	if len(svc.Status.LoadBalancer.Ingress) > 0 {
-		t.Errorf("Expected LoadBalancer status to be empty, Got %v", svc.Status.LoadBalancer)
-	}
-
-	expectedAnnotationsKeys := calculateExpectedAnnotationsKeys(svc)
-	var missingKeys []string
-	for _, key := range expectedAnnotationsKeys {
-		if _, ok := svc.Annotations[key]; !ok {
-			missingKeys = append(missingKeys, key)
-		}
-	}
-	if len(missingKeys) != len(expectedAnnotationsKeys) {
-		t.Errorf("Unexpected ILB annotations present, Got %v", svc.Annotations)
-	}
-}
-
-func createLegacyForwardingRule(t *testing.T, svc *api_v1.Service, cloud *gce.Cloud, scheme string) {
-	t.Helper()
-	frName := cloudprovider.DefaultLoadBalancerName(svc)
-	key, err := composite.CreateKey(cloud, frName, meta.Regional)
-	if err != nil {
-		t.Errorf("Unexpected error when creating key - %v", err)
-	}
-	var ip string
-	if len(svc.Status.LoadBalancer.Ingress) > 0 {
-		ip = svc.Status.LoadBalancer.Ingress[0].IP
-	}
-	existingFwdRule := &composite.ForwardingRule{
-		Name:                frName,
-		IPAddress:           ip,
-		Ports:               []string{"123"},
-		IPProtocol:          "TCP",
-		LoadBalancingScheme: scheme,
-	}
-	if err = composite.CreateForwardingRule(cloud, key, existingFwdRule); err != nil {
-		t.Errorf("Failed to create fake forwarding rule %s, err %v", frName, err)
-	}
-}
-
 // TestProcessCreateOrUpdate verifies the processing loop in L4Controller.
 // This test adds a new service, then performs a valid update and then modifies the service type to External and ensures
 // that the status field is as expected in each case.
@@ -768,3 +614,188 @@ func TestCreateDeleteDualStackService(t *testing.T) {
 		})
 	}
 }
+
+func TestProcessDualStackServiceOnUserError(t *testing.T) {
+	t.Parallel()
+	l4c := newServiceController(t, newFakeGCEWithUserNoIPv6SubnetError())
+	l4c.enableDualStack = true
+
+	newSvc := test.NewL4ILBDualStackService(8080, api_v1.ProtocolTCP, []api_v1.IPFamily{api_v1.IPv4Protocol, api_v1.IPv6Protocol}, api_v1.ServiceExternalTrafficPolicyTypeCluster)
+	addILBService(l4c, newSvc)
+	addNEG(l4c, newSvc)
+	syncResult := l4c.processServiceCreateOrUpdate(newSvc)
+	if syncResult.Error == nil {
+		t.Fatalf("Failed to generate error when syncing service %s", newSvc.Name)
+	}
+	if !syncResult.MetricsState.IsUserError {
+		t.Errorf("syncResult.MetricsState.IsUserError should be true, got false")
+	}
+	if syncResult.MetricsState.InSuccess {
+		t.Errorf("syncResult.MetricsState.InSuccess should be false, got true")
+	}
+	if syncResult.DualStackMetricsState.Status != metrics.StatusUserError {
+		t.Errorf("syncResult.DualStackMetricsState.Status should be %s, got %s", metrics.StatusUserError, syncResult.DualStackMetricsState.Status)
+	}
+}
+
+func newServiceController(t *testing.T, fakeGCE *gce.Cloud) *L4Controller {
+	kubeClient := fake.NewSimpleClientset()
+
+	vals := gce.DefaultTestClusterValues()
+	namer := namer.NewNamer(clusterUID, "")
+
+	stopCh := make(chan struct{})
+	ctxConfig := context.ControllerContextConfig{
+		Namespace:    api_v1.NamespaceAll,
+		ResyncPeriod: 1 * time.Minute,
+		NumL4Workers: 5,
+	}
+	ctx := context.NewControllerContext(nil, kubeClient, nil, nil, nil, nil, nil, fakeGCE, namer, "" /*kubeSystemUID*/, ctxConfig)
+	// Add some nodes so that NEG linker kicks in during ILB creation.
+	nodes, err := test.CreateAndInsertNodes(ctx.Cloud, []string{"instance-1"}, vals.ZoneName)
+	if err != nil {
+		t.Errorf("Failed to add new nodes, err  %v", err)
+	}
+	for _, n := range nodes {
+		ctx.NodeInformer.GetIndexer().Add(n)
+	}
+	return NewILBController(ctx, stopCh)
+}
+
+func newFakeGCE() *gce.Cloud {
+	vals := gce.DefaultTestClusterValues()
+	fakeGCE := gce.NewFakeGCECloud(vals)
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = loadbalancers.InsertForwardingRuleHook
+	return fakeGCE
+}
+
+func newFakeGCEWithInsertError() *gce.Cloud {
+	vals := gce.DefaultTestClusterValues()
+	fakeGCE := gce.NewFakeGCECloud(vals)
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = mock.InsertForwardingRulesInternalErrHook
+	return fakeGCE
+}
+
+func newFakeGCEWithUserInsertError() *gce.Cloud {
+	vals := gce.DefaultTestClusterValues()
+	fakeGCE := gce.NewFakeGCECloud(vals)
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = test.InsertForwardingRuleErrorHook(&googleapi.Error{Code: http.StatusConflict, Message: "IP_IN_USE_BY_ANOTHER_RESOURCE - IP '1.1.1.1' is already being used by another resource."})
+	return fakeGCE
+}
+
+func newFakeGCEWithUserNoIPv6SubnetError() *gce.Cloud {
+	vals := gce.DefaultTestClusterValues()
+	fakeGCE := gce.NewFakeGCECloud(vals)
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockForwardingRules.InsertHook = test.InsertForwardingRuleErrorHook(&googleapi.Error{Code: http.StatusBadRequest, Message: "Subnetwork does not have an internal IPv6 IP space which is required for IPv6 L4 ILB forwarding rules."})
+	return fakeGCE
+}
+
+func addILBService(l4c *L4Controller, svc *api_v1.Service) {
+	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Create(context2.TODO(), svc, v1.CreateOptions{})
+	l4c.ctx.ServiceInformer.GetIndexer().Add(svc)
+}
+
+func updateILBService(l4c *L4Controller, svc *api_v1.Service) {
+	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Update(context2.TODO(), svc, v1.UpdateOptions{})
+	l4c.ctx.ServiceInformer.GetIndexer().Update(svc)
+}
+
+func deleteILBService(l4c *L4Controller, svc *api_v1.Service) {
+	l4c.ctx.KubeClient.CoreV1().Services(svc.Namespace).Delete(context2.TODO(), svc.Name, v1.DeleteOptions{})
+	l4c.ctx.ServiceInformer.GetIndexer().Delete(svc)
+}
+
+func addNEG(l4c *L4Controller, svc *api_v1.Service) {
+	// Also create a fake NEG for this service since the sync code will try to link the backend service to NEG
+	negName := l4c.namer.L4Backend(svc.Namespace, svc.Name)
+	neg := &composite.NetworkEndpointGroup{Name: negName}
+	key := meta.ZonalKey(negName, testGCEZone)
+	composite.CreateNetworkEndpointGroup(l4c.ctx.Cloud, key, neg)
+}
+
+func getKeyForSvc(svc *api_v1.Service, t *testing.T) string {
+	key, err := common.KeyFunc(svc)
+	if err != nil {
+		t.Fatalf("Failed to get key for service %v, err : %v", svc, err)
+	}
+	return key
+}
+
+func calculateExpectedAnnotationsKeys(svc *api_v1.Service) []string {
+	expectedAnnotations := ilbCommonAnnotationKeys
+	if utils.NeedsIPv4(svc) {
+		expectedAnnotations = append(expectedAnnotations, ilbIPv4AnnotationKeys...)
+	}
+	if utils.NeedsIPv6(svc) {
+		expectedAnnotations = append(expectedAnnotations, ilbIPv6AnnotationKeys...)
+	}
+	return expectedAnnotations
+}
+
+func verifyILBServiceProvisioned(t *testing.T, svc *api_v1.Service) {
+	t.Helper()
+
+	if !common.HasGivenFinalizer(svc.ObjectMeta, common.ILBFinalizerV2) {
+		t.Errorf("ILB v2 finalizer is not found, expected to exist, svc %+v", svc)
+	}
+	if len(svc.Status.LoadBalancer.Ingress) == 0 || svc.Status.LoadBalancer.Ingress[0].IP == "" {
+		t.Errorf("Invalid LoadBalancer status field in service - %+v", svc.Status.LoadBalancer)
+	}
+
+	expectedAnnotationsKeys := calculateExpectedAnnotationsKeys(svc)
+	var missingKeys []string
+	for _, key := range expectedAnnotationsKeys {
+		if _, ok := svc.Annotations[key]; !ok {
+			missingKeys = append(missingKeys, key)
+		}
+	}
+	if len(missingKeys) > 0 {
+		t.Errorf("Cannot find annotations %v in ILB service, Got %v", missingKeys, svc.Annotations)
+	}
+}
+
+func verifyILBServiceNotProvisioned(t *testing.T, svc *api_v1.Service) {
+	t.Helper()
+
+	if common.HasGivenFinalizer(svc.ObjectMeta, common.ILBFinalizerV2) {
+		t.Errorf("ILB v2 finalizer should not exist on service %+v", svc)
+	}
+
+	if len(svc.Status.LoadBalancer.Ingress) > 0 {
+		t.Errorf("Expected LoadBalancer status to be empty, Got %v", svc.Status.LoadBalancer)
+	}
+
+	expectedAnnotationsKeys := calculateExpectedAnnotationsKeys(svc)
+	var missingKeys []string
+	for _, key := range expectedAnnotationsKeys {
+		if _, ok := svc.Annotations[key]; !ok {
+			missingKeys = append(missingKeys, key)
+		}
+	}
+	if len(missingKeys) != len(expectedAnnotationsKeys) {
+		t.Errorf("Unexpected ILB annotations present, Got %v", svc.Annotations)
+	}
+}
+
+func createLegacyForwardingRule(t *testing.T, svc *api_v1.Service, cloud *gce.Cloud, scheme string) {
+	t.Helper()
+	frName := cloudprovider.DefaultLoadBalancerName(svc)
+	key, err := composite.CreateKey(cloud, frName, meta.Regional)
+	if err != nil {
+		t.Errorf("Unexpected error when creating key - %v", err)
+	}
+	var ip string
+	if len(svc.Status.LoadBalancer.Ingress) > 0 {
+		ip = svc.Status.LoadBalancer.Ingress[0].IP
+	}
+	existingFwdRule := &composite.ForwardingRule{
+		Name:                frName,
+		IPAddress:           ip,
+		Ports:               []string{"123"},
+		IPProtocol:          "TCP",
+		LoadBalancingScheme: scheme,
+	}
+	if err = composite.CreateForwardingRule(cloud, key, existingFwdRule); err != nil {
+		t.Errorf("Failed to create fake forwarding rule %s, err %v", frName, err)
+	}
+}

pkg/metrics/types.go

@@ -75,8 +75,9 @@ type L4ILBServiceState struct {
 
 type L4ILBDualStackServiceStateStatus string
 
-var StatusSuccess = L4ILBDualStackServiceStateStatus("Success")
-var StatusError = L4ILBDualStackServiceStateStatus("Error")
+const StatusSuccess = L4ILBDualStackServiceStateStatus("Success")
+const StatusUserError = L4ILBDualStackServiceStateStatus("UserError")
+const StatusError = L4ILBDualStackServiceStateStatus("Error")
 
 // L4ILBDualStackServiceState defines ipFamilies, ipFamilyPolicy and status
 // of L4 ILB DualStack service