Skip to content

Commit

Permalink
Merge pull request #1905 from panslava/ipv6-user-errors
Browse files Browse the repository at this point in the history 
Add UserError Status to ILB DualStack metrics
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jan 26, 2023
2 parents f3c56e7 + 4f5f046 commit 103305b
Show file tree
Hide file tree
Showing 3 changed files with 194 additions and 158 deletions.
6 changes: 5 additions & 1 deletion pkg/l4lb/l4controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
}
Expand Down
341 changes: 186 additions & 155 deletions pkg/l4lb/l4controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}
5 changes: 3 additions & 2 deletions pkg/metrics/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 103305b

Please sign in to comment.