From 5e6bb4c1c5cdc74daf34185a90596f0e905367b3 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Fri, 6 Sep 2019 16:03:49 -0700 Subject: [PATCH] Add supplementary e2e tests for Finalizer --- cmd/e2e-test/finalizer_test.go | 234 +++++++++++++++++++++++++-------- pkg/e2e/helpers.go | 24 +++- pkg/fuzz/gcp.go | 30 +++-- 3 files changed, 222 insertions(+), 66 deletions(-) diff --git a/cmd/e2e-test/finalizer_test.go b/cmd/e2e-test/finalizer_test.go index 51a3417b57..a57346e322 100644 --- a/cmd/e2e-test/finalizer_test.go +++ b/cmd/e2e-test/finalizer_test.go @@ -20,6 +20,7 @@ import ( "context" "testing" + "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/ingress-gce/pkg/e2e" "k8s.io/ingress-gce/pkg/fuzz" @@ -27,134 +28,261 @@ import ( "k8s.io/ingress-gce/pkg/utils" ) +// TestFinalizer asserts that finalizer is added/ removed appropriately during the life cycle +// of an ingress resource. func TestFinalizer(t *testing.T) { t.Parallel() ctx := context.Background() port80 := intstr.FromInt(80) + numForwardingRules := 1 + numBackendServices := 2 + svcName := "service-1" ing := fuzz.NewIngressBuilder("", "ingress-1", ""). - AddPath("foo.com", "/", "service-1", port80). + AddPath("foo.com", "/", svcName, port80). SetIngressClass("gce"). Build() Framework.RunWithSandbox("finalizer", t, func(t *testing.T, s *e2e.Sandbox) { - _, err := e2e.CreateEchoService(s, "service-1", nil) + _, err := e2e.CreateEchoService(s, svcName, nil) if err != nil { - t.Fatalf("error creating echo service: %v", err) + t.Fatalf("CreateEchoService(_, %q, nil): %v, want nil", svcName, err) } - t.Logf("Echo service created (%s/%s)", s.Namespace, "service-1") + t.Logf("Echo service created (%s/%s)", s.Namespace, svcName) crud := e2e.IngressCRUD{C: Framework.Clientset} ing.Namespace = s.Namespace if _, err := crud.Create(ing); err != nil { - t.Fatalf("error creating Ingress spec: %v", err) + t.Fatalf("create(%s/%s) = %v, want nil; Ingress: %v", ing.Namespace, ing.Name, err, ing) } t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) + ing = waitForStableIngress(true, ing, s, t) - ing, err := e2e.WaitForIngress(s, ing, nil) + ingFinalizers := ing.GetFinalizers() + if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey { + t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey) + } + + gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices) + + deleteOptions := &fuzz.GCLBDeleteOptions{ + SkipDefaultBackend: true, + } + if err := e2e.WaitForIngressDeletion(ctx, gclb, s, ing, deleteOptions); err != nil { + t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) + } + }) +} + +// TestFinalizerIngressClassChange asserts that finalizer is removed from ingress after its +// associated resources are cleaned up when ingress class is updated to a non glbc type. +func TestFinalizerIngressClassChange(t *testing.T) { + t.Parallel() + ctx := context.Background() + port80 := intstr.FromInt(80) + numForwardingRules := 1 + numBackendServices := 2 + svcName := "service-1" + ing := fuzz.NewIngressBuilder("", "ingress-1", ""). + AddPath("foo.com", "/", svcName, port80). + SetIngressClass("gce"). + Build() + + Framework.RunWithSandbox("finalizer-ingress-class-change", t, func(t *testing.T, s *e2e.Sandbox) { + _, err := e2e.CreateEchoService(s, svcName, nil) if err != nil { - t.Fatalf("error waiting for Ingress to stabilize: %v", err) + t.Fatalf("CreateEchoService(_, %q, nil): %v, want nil", svcName, err) } - t.Logf("GCLB resources created (%s/%s)", s.Namespace, ing.Name) + t.Logf("Echo service created (%s/%s)", s.Namespace, svcName) - // Perform whitebox testing. - if len(ing.Status.LoadBalancer.Ingress) < 1 { - t.Fatalf("Ingress does not have an IP: %+v", ing.Status) + crud := e2e.IngressCRUD{C: Framework.Clientset} + ing.Namespace = s.Namespace + if _, err := crud.Create(ing); err != nil { + t.Fatalf("create(%s/%s) = %v, want nil; Ingress: %v", ing.Namespace, ing.Name, err, ing) } + t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) + ing = waitForStableIngress(true, ing, s, t) ingFinalizers := ing.GetFinalizers() if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey { - t.Fatalf("GetFinalizers() = %+v, want [%s]", ingFinalizers, utils.FinalizerKey) + t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey) } - vip := ing.Status.LoadBalancer.Ingress[0].IP - t.Logf("Ingress %s/%s VIP = %s", s.Namespace, ing.Name, vip) - gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) - if err != nil { - t.Fatalf("Error getting GCP resources for LB with IP = %q: %v", vip, err) - } + gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices) + + // Change Ingress class + newIngClass := "nginx" + ing = fuzz.NewIngressBuilderFromExisting(ing).SetIngressClass(newIngClass).Build() - if err = e2e.CheckGCLB(gclb, 1, 2); err != nil { - t.Error(err) + if _, err := crud.Update(ing); err != nil { + t.Fatalf("update(%s/%s) = %v, want nil; ingress: %v", ing.Namespace, ing.Name, err, ing) } + t.Logf("Ingress (%s/%s) class changed to %s", s.Namespace, ing.Name, newIngClass) deleteOptions := &fuzz.GCLBDeleteOptions{ SkipDefaultBackend: true, } - if err := e2e.WaitForIngressDeletion(ctx, gclb, s, ing, deleteOptions); err != nil { - t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) + if err := e2e.WaitForFinalizerDeletion(ctx, gclb, s, ing.Name, deleteOptions); err != nil { + t.Errorf("e2e.WaitForFinalizerDeletion(..., %q, _) = %v, want nil", ing.Name, err) } + t.Logf("Finalizer for Ingress (%s/%s) deleted", s.Namespace, ing.Name) }) } -func TestFinalizerIngressClassChange(t *testing.T) { +// TestFinalizerIngressesWithSharedResources asserts that sync does not return error with finalizer +// when multiple ingresses with shared resources are created or deleted. +func TestFinalizerIngressesWithSharedResources(t *testing.T) { t.Parallel() ctx := context.Background() port80 := intstr.FromInt(80) + numForwardingRules := 1 + numBackendServices := 2 + svcName := "service-1" ing := fuzz.NewIngressBuilder("", "ingress-1", ""). - AddPath("foo.com", "/", "service-1", port80). + AddPath("foo.com", "/", svcName, port80). + SetIngressClass("gce"). + Build() + otherIng := fuzz.NewIngressBuilder("", "ingress-2", ""). + AddPath("foo.com", "/", svcName, port80). SetIngressClass("gce"). Build() Framework.RunWithSandbox("finalizer-ingress-class-change", t, func(t *testing.T, s *e2e.Sandbox) { - _, err := e2e.CreateEchoService(s, "service-1", nil) + _, err := e2e.CreateEchoService(s, svcName, nil) if err != nil { - t.Fatalf("error creating echo service: %v", err) + t.Fatalf("CreateEchoService(_, %q, nil): %v, want nil", svcName, err) } - t.Logf("Echo service created (%s/%s)", s.Namespace, "service-1") + t.Logf("Echo service created (%s/%s)", s.Namespace, svcName) crud := e2e.IngressCRUD{C: Framework.Clientset} ing.Namespace = s.Namespace if _, err := crud.Create(ing); err != nil { - t.Fatalf("error creating Ingress spec: %v", err) + t.Fatalf("create(%s/%s) = %v, want nil; Ingress: %v", ing.Namespace, ing.Name, err, ing) } t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) - ing, err := e2e.WaitForIngress(s, ing, nil) - if err != nil { - t.Fatalf("error waiting for Ingress to stabilize: %v", err) + otherIng.Namespace = s.Namespace + if _, err := crud.Create(otherIng); err != nil { + t.Fatalf("create(%s/%s) = %v, want nil; Ingress: %v", otherIng.Namespace, otherIng.Name, err, otherIng) } - t.Logf("GCLB resources created (%s/%s)", s.Namespace, ing.Name) + t.Logf("Ingress created (%s/%s)", s.Namespace, otherIng.Name) + + ing = waitForStableIngress(true, ing, s, t) + otherIng = waitForStableIngress(true, otherIng, s, t) ingFinalizers := ing.GetFinalizers() if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey { t.Fatalf("GetFinalizers() = %+v, want [%q]", ingFinalizers, utils.FinalizerKey) } + otherIngFinalizers := otherIng.GetFinalizers() + if len(otherIngFinalizers) != 1 || otherIngFinalizers[0] != utils.FinalizerKey { + t.Fatalf("GetFinalizers() = %+v, want [%q]", otherIngFinalizers, utils.FinalizerKey) + } - // Perform whitebox testing. - if len(ing.Status.LoadBalancer.Ingress) < 1 { - t.Fatalf("Ingress does not have an IP: %+v", ing.Status) + gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices) + otherGclb := checkGCLB(t, s, otherIng, numForwardingRules, numBackendServices) + + // SkipBackends ensure that we dont wait on deletion of shared backends. + deleteOptions := &fuzz.GCLBDeleteOptions{ + SkipDefaultBackend: true, + SkipBackends: true, + } + if err := e2e.WaitForIngressDeletion(ctx, gclb, s, ing, deleteOptions); err != nil { + t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) + } + deleteOptions = &fuzz.GCLBDeleteOptions{ + SkipDefaultBackend: true, } + if err := e2e.WaitForIngressDeletion(ctx, otherGclb, s, otherIng, deleteOptions); err != nil { + t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", otherIng.Name, err) + } + }) +} - vip := ing.Status.LoadBalancer.Ingress[0].IP - t.Logf("Ingress %s/%s VIP = %s", s.Namespace, ing.Name, vip) - gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) +// TestUpdateTo1dot7 asserts that finalizer is added to an ingress when upgraded from a version +// without finalizer to version 1.7. Also, verifies that ingress is deleted with finalizer enabled. +// Note: The test is named in such a way that it does run as a normal test or an upgrade test for +// other versions. +func TestUpdateTo1dot7(t *testing.T) { + port80 := intstr.FromInt(80) + numForwardingRules := 1 + numBackendServices := 2 + svcName := "service-1" + ing := fuzz.NewIngressBuilder("", "ingress-1", ""). + AddPath("foo.com", "/", svcName, port80). + SetIngressClass("gce"). + Build() + Framework.RunWithSandbox("finalizer-master-upgrade", t, func(t *testing.T, s *e2e.Sandbox) { + t.Parallel() + + _, err := e2e.CreateEchoService(s, svcName, nil) if err != nil { - t.Fatalf("Error getting GCP resources for LB with IP = %q: %v", vip, err) + t.Fatalf("CreateEchoService(_, %q, nil): %v, want nil", svcName, err) } + t.Logf("Echo service created (%s/%s)", s.Namespace, svcName) - if err = e2e.CheckGCLB(gclb, 1, 2); err != nil { - t.Error(err) + crud := e2e.IngressCRUD{C: Framework.Clientset} + ing.Namespace = s.Namespace + if _, err := crud.Create(ing); err != nil { + t.Fatalf("create(%s/%s) = %v, want nil; Ingress: %v", ing.Namespace, ing.Name, err, ing) } + t.Logf("Ingress created (%s/%s)", s.Namespace, ing.Name) + waitForStableIngress(true, ing, s, t) - // Change Ingress class - newIngClass := "nginx" - ing = fuzz.NewIngressBuilderFromExisting(ing).SetIngressClass(newIngClass).Build() + // Check that finalizer is not added in old version in which finalizer add is not enabled. + ingFinalizers := ing.GetFinalizers() + if l := len(ingFinalizers); l != 0 { + t.Fatalf("GetFinalizers() = %d, want 0", l) + } - if _, err := crud.Update(ing); err != nil { - t.Fatalf("error updating Ingress spec: %v", err) + checkGCLB(t, s, ing, numForwardingRules, numBackendServices) + + for { + // While k8s master is upgrading, it will return a connection refused + // error for any k8s resource we try to hit. We loop until the + // master upgrade has finished. + if s.MasterUpgrading() { + continue + } + + if s.MasterUpgraded() { + t.Logf("Detected master upgrade") + break + } } - t.Logf("Ingress (%s/%s) class changed to %s", s.Namespace, ing.Name, newIngClass) + // Wait for finalizer to be added and verify that correct finalizer is added to the ingress after the upgrade. + if err := e2e.WaitForFinalizer(s, ing.Name); err != nil { + t.Errorf("e2e.WaitForFinalizer(_, %q) = %v, want nil", ing.Name, err) + } + + gclb := checkGCLB(t, s, ing, numForwardingRules, numBackendServices) + + // If the Master has upgraded and the Ingress is stable, + // we delete the Ingress and exit out of the loop to indicate that + // the test is done. deleteOptions := &fuzz.GCLBDeleteOptions{ SkipDefaultBackend: true, } - if err := e2e.WaitForFinalizerDeletion(ctx, gclb, s, ing.Name, deleteOptions); err != nil { - t.Errorf("e2e.WaitForFinalizerDeletion(...) = %v, want nil", err) - } - t.Logf("Finalizer for Ingress (%s/%s) deleted", s.Namespace, ing.Name) - - if err := e2e.WaitForIngressDeletion(ctx, gclb, s, ing, deleteOptions); err != nil { + if err := e2e.WaitForIngressDeletion(context.Background(), gclb, s, ing, deleteOptions); err != nil { t.Errorf("e2e.WaitForIngressDeletion(..., %q, nil) = %v, want nil", ing.Name, err) } }) } + +func checkGCLB(t *testing.T, s *e2e.Sandbox, ing *v1beta1.Ingress, numForwardingRules, numBackendServices int) *fuzz.GCLB { + // Perform whitebox testing. + if len(ing.Status.LoadBalancer.Ingress) < 1 { + t.Fatalf("Ingress does not have an IP: %+v", ing.Status) + } + vip := ing.Status.LoadBalancer.Ingress[0].IP + t.Logf("Ingress %s/%s VIP = %s", s.Namespace, ing.Name, vip) + gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)) + if err != nil { + t.Fatalf("GCLBForVIP(..., %q, _) = %v, want nil; error getting GCP resources for LB with IP", vip, err) + } + + if err = e2e.CheckGCLB(gclb, numForwardingRules, numBackendServices); err != nil { + t.Error(err) + } + return gclb +} diff --git a/pkg/e2e/helpers.go b/pkg/e2e/helpers.go index f437f10451..f7f465f51c 100644 --- a/pkg/e2e/helpers.go +++ b/pkg/e2e/helpers.go @@ -37,6 +37,7 @@ import ( "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/fuzz" "k8s.io/ingress-gce/pkg/fuzz/features" + "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog" "net/http" "strings" @@ -114,6 +115,27 @@ func WaitForIngress(s *Sandbox, ing *v1beta1.Ingress, options *WaitForIngressOpt return ing, err } +// WaitForFinalizer waits for Finalizer to be added. +// This is used when master is upgraded from version without finalizer. +// We wait for new lb controller to add finalizer. +func WaitForFinalizer(s *Sandbox, ingName string) error { + crud := IngressCRUD{s.f.Clientset} + klog.Infof("Waiting for Finalizer to be added for Ingress %s/%s", s.Namespace, ingName) + return wait.Poll(k8sApiPoolInterval, k8sApiPollTimeout, func() (bool, error) { + ing, err := crud.Get(s.Namespace, ingName) + if err != nil { + klog.Infof("WaitForFinalizer(%s/%s) = %v, error retrieving Ingress", s.Namespace, ingName, err) + return false, nil + } + ingFinalizers := ing.GetFinalizers() + if len(ingFinalizers) != 1 || ingFinalizers[0] != utils.FinalizerKey { + klog.Infof("WaitForFinalizer(%s/%s) = %v, finalizer not added for Ingress %v", s.Namespace, ingName, ingFinalizers, ing) + return false, nil + } + return true, nil + }) +} + // WaitForIngressDeletion deletes the given ingress and waits for the // resources associated with it to be deleted. func WaitForIngressDeletion(ctx context.Context, g *fuzz.GCLB, s *Sandbox, ing *v1beta1.Ingress, options *fuzz.GCLBDeleteOptions) error { @@ -143,7 +165,7 @@ func WaitForFinalizerDeletion(ctx context.Context, g *fuzz.GCLB, s *Sandbox, ing return wait.Poll(k8sApiPoolInterval, k8sApiPollTimeout, func() (bool, error) { ing, err := crud.Get(s.Namespace, ingName) if err != nil { - klog.Infof("WaitForFinalizerDeletion(%s/%s) = Error retrieving Ingress: %v", s.Namespace, ing.Name, err) + klog.Infof("WaitForFinalizerDeletion(%s/%s) = Error retrieving Ingress: %v", s.Namespace, ingName, err) return false, nil } if len(ing.GetFinalizers()) != 0 { diff --git a/pkg/fuzz/gcp.go b/pkg/fuzz/gcp.go index 3d2b22bfb6..bf6a6417e4 100644 --- a/pkg/fuzz/gcp.go +++ b/pkg/fuzz/gcp.go @@ -126,6 +126,10 @@ type GCLBDeleteOptions struct { // SkipDefaultBackend indicates whether to skip checking for the // system default backend. SkipDefaultBackend bool + // SkipBackends indicates whether to skip checking for the backends. + // This is enabled only when we know that backends are shared among multiple ingresses + // in which case shared backends are not cleaned up on ingress deletion. + SkipBackends bool } // CheckResourceDeletion checks the existence of the resources. Returns nil if @@ -178,20 +182,22 @@ func (g *GCLB) CheckResourceDeletion(ctx context.Context, c cloud.Cloud, options resources = append(resources, k) } } - for k := range g.BackendService { - bs, err := c.BackendServices().Get(ctx, &k) - if err != nil { - if err.(*googleapi.Error) == nil || err.(*googleapi.Error).Code != http.StatusNotFound { - return fmt.Errorf("BackendService %s is not deleted/error to get: %s", k.Name, err) - } - } else { - if options != nil && options.SkipDefaultBackend { - desc := utils.DescriptionFromString(bs.Description) - if desc.ServiceName == "kube-system/default-http-backend" { - continue + if options == nil || !options.SkipBackends { + for k := range g.BackendService { + bs, err := c.BackendServices().Get(ctx, &k) + if err != nil { + if err.(*googleapi.Error) == nil || err.(*googleapi.Error).Code != http.StatusNotFound { + return fmt.Errorf("BackendService %s is not deleted/error to get: %s", k.Name, err) + } + } else { + if options != nil && options.SkipDefaultBackend { + desc := utils.DescriptionFromString(bs.Description) + if desc.ServiceName == "kube-system/default-http-backend" { + continue + } } + resources = append(resources, k) } - resources = append(resources, k) } } for k := range g.NetworkEndpointGroup {