Merge pull request #905 from skmatti/resource-leak-fix-e2e

Add e2e tests for frontend resource leak fix

cmd/e2e-test/basic_test.go

@@ -239,3 +239,92 @@ func whiteboxTest(ing *v1beta1.Ingress, s *e2e.Sandbox, t *testing.T, region str
 	}
 	return gclb
 }
+
+// TestFrontendResourceDeletion asserts that unused GCP frontend resources are
+// deleted. This also tests that necessary GCP frontend resources exist.
+func TestFrontendResourceDeletion(t *testing.T) {
+	t.Parallel()
+	port80 := intstr.FromInt(80)
+	svcName := "service-1"
+	host := "foo.com"
+
+	// All the test cases create an ingress with a HTTP + HTTPS load-balancer
+	// at the beginning of the test and turnoff these based on the testcase params.
+	for _, tc := range []struct {
+		desc         string
+		disableHTTP  bool
+		disableHTTPS bool
+	}{
+		// Note that disabling both HTTP & HTTPS would result in a sync error, so excluded.
+		{"http only", false, true},
+		{"https only", true, false},
+	} {
+		tc := tc
+		Framework.RunWithSandbox(tc.desc, t, func(t *testing.T, s *e2e.Sandbox) {
+			t.Parallel()
+			ctx := context.Background()
+
+			_, err := e2e.CreateEchoService(s, svcName, nil)
+			if err != nil {
+				t.Fatalf("CreateEchoService(_, %q, nil): %v, want nil", svcName, err)
+			}
+			t.Logf("Echo service created (%s/%s)", s.Namespace, svcName)
+
+			// Create SSL certificate.
+			certName := fmt.Sprintf("cert-1")
+			cert, err := e2e.NewCert(certName, host, e2e.K8sCert, false)
+			if err != nil {
+				t.Fatalf("e2e.NewCert(%q, %q, _, %t) = %v, want nil", certName, host, false, err)
+			}
+			if err := cert.Create(s); err != nil {
+				t.Fatalf("cert.Create(_) = %v, want nil, error creating cert %s", err, cert.Name)
+			}
+			t.Logf("Cert created %s", certName)
+			defer cert.Delete(s)
+
+			ing := fuzz.NewIngressBuilder(s.Namespace, "ing1", "").
+				AddPath(host, "/", svcName, port80).AddTLS([]string{}, cert.Name).Build()
+
+			crud := e2e.IngressCRUD{C: Framework.Clientset}
+			if _, err := crud.Create(ing); err != nil {
+				t.Fatalf("crud.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)
+			gclb := whiteboxTest(ing, s, t, "")
+
+			// Update ingress with desired frontend resource configuration.
+			ingBuilder := fuzz.NewIngressBuilderFromExisting(ing)
+			if tc.disableHTTP {
+				ingBuilder = ingBuilder.SetAllowHttp(false)
+			}
+			if tc.disableHTTPS {
+				ingBuilder = ingBuilder.SetTLS(nil)
+			}
+			ing = ingBuilder.Build()
+
+			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 updated (%s/%s)", ing.Namespace, ing.Name)
+			ing = waitForStableIngress(true, ing, s, t)
+
+			deleteOptions := &fuzz.GCLBDeleteOptions{
+				SkipDefaultBackend:          true,
+				CheckHttpFrontendResources:  tc.disableHTTP,
+				CheckHttpsFrontendResources: tc.disableHTTPS,
+			}
+			// Wait for unused frontend resources to be deleted.
+			if err := e2e.WaitForFrontendResourceDeletion(ctx, Framework.Cloud, gclb, deleteOptions); err != nil {
+				t.Errorf("e2e.WaitForIngressDeletion(..., %q, _) = %v, want nil", ing.Name, err)
+			}
+			whiteboxTest(ing, s, t, "")
+			deleteOptions = &fuzz.GCLBDeleteOptions{
+				SkipDefaultBackend: true,
+			}
+			if err := e2e.WaitForIngressDeletion(ctx, gclb, s, ing, deleteOptions); err != nil {
+				t.Errorf("e2e.WaitForIngressDeletion(..., %q, _) = %v, want nil", ing.Name, err)
+			}
+		})
+	}
+}

pkg/e2e/fixtures.go

@@ -222,7 +222,7 @@ func DeleteSecret(s *Sandbox, name string) error {
 	if err := s.f.Clientset.CoreV1().Secrets(s.Namespace).Delete(name, &metav1.DeleteOptions{}); err != nil {
 		return err
 	}
-	klog.V(2).Infof("Secret %q:%q created", s.Namespace, name)
+	klog.V(2).Infof("Secret %q:%q deleted", s.Namespace, name)
 
 	return nil
 }

pkg/e2e/helpers.go

@@ -201,6 +201,26 @@ func WaitForGCLBDeletion(ctx context.Context, c cloud.Cloud, g *fuzz.GCLB, optio
 	})
 }
 
+// WaitForFrontendResourceDeletion waits for frontend resources associated with the GLBC to be
+// deleted for given protocol.
+func WaitForFrontendResourceDeletion(ctx context.Context, c cloud.Cloud, g *fuzz.GCLB, options *fuzz.GCLBDeleteOptions) error {
+	return wait.Poll(gclbDeletionInterval, gclbDeletionTimeout, func() (bool, error) {
+		if options.CheckHttpFrontendResources {
+			if err := g.CheckResourceDeletionByProtocol(ctx, c, options, fuzz.HttpProtocol); err != nil {
+				klog.Infof("WaitForGCLBDeletionByProtocol(..., %q, %q) = %v", g.VIP, fuzz.HttpsProtocol, err)
+				return false, nil
+			}
+		}
+		if options.CheckHttpsFrontendResources {
+			if err := g.CheckResourceDeletionByProtocol(ctx, c, options, fuzz.HttpsProtocol); err != nil {
+				klog.Infof("WaitForGCLBDeletionByProtocol(..., %q, %q) = %v", g.VIP, fuzz.HttpsProtocol, err)
+				return false, nil
+			}
+		}
+		return true, nil
+	})
+}
+
 // WaitForNEGDeletion waits for all NEGs associated with a GCLB to be deleted via GC
 func WaitForNEGDeletion(ctx context.Context, c cloud.Cloud, g *fuzz.GCLB, options *fuzz.GCLBDeleteOptions) error {
 	return wait.Poll(negPollInterval, gclbDeletionTimeout, func() (bool, error) {

pkg/fuzz/gcp.go

@@ -37,10 +37,17 @@ import (
 )
 
 const (
-	NegResourceType = "networkEndpointGroup"
-	IgResourceType  = "instanceGroup"
+	NegResourceType          = "networkEndpointGroup"
+	IgResourceType           = "instanceGroup"
+	HttpProtocol             = Protocol("HTTP")
+	HttpsProtocol            = Protocol("HTTPS")
+	targetHTTPProxyResource  = "targetHttpProxies"
+	targetHTTPSProxyResource = "targetHttpsProxies"
 )
 
+// Protocol specifies GCE loadbalancer protocol.
+type Protocol string
+
 // ForwardingRule is a union of the API version types.
 type ForwardingRule struct {
 	GA    *compute.ForwardingRule
@@ -130,6 +137,12 @@ type GCLBDeleteOptions struct {
 	// 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
+	// CheckHttpFrontendResources indicates whether to check just the http
+	// frontend resources.
+	CheckHttpFrontendResources bool
+	// CheckHttpsFrontendResources indicates whether to check just the https
+	// frontend resources.
+	CheckHttpsFrontendResources bool
 }
 
 // CheckResourceDeletion checks the existence of the resources. Returns nil if
@@ -245,7 +258,89 @@ func (g *GCLB) CheckResourceDeletion(ctx context.Context, c cloud.Cloud, options
 	return nil
 }
 
-// CheckNEGDeletion checks all NEGs associated with the GCLB have been deleted
+// CheckResourceDeletionByProtocol checks the existence of the resources for given protocol.
+// Returns nil if all of the associated frontend resources no longer exist.
+func (g *GCLB) CheckResourceDeletionByProtocol(ctx context.Context, c cloud.Cloud, options *GCLBDeleteOptions, protocol Protocol) error {
+	var resources []meta.Key
+
+	for k, gfr := range g.ForwardingRule {
+		// Check if forwarding rule matches given protocol.
+		if gfrProtocol, err := getForwardingRuleProtocol(gfr.GA); err != nil {
+			return err
+		} else if gfrProtocol != protocol {
+			continue
+		}
+
+		var err error
+		if k.Region != "" {
+			_, err = c.ForwardingRules().Get(ctx, &k)
+		} else {
+			_, err = c.GlobalForwardingRules().Get(ctx, &k)
+		}
+		if err != nil {
+			if err.(*googleapi.Error) == nil || err.(*googleapi.Error).Code != http.StatusNotFound {
+				return fmt.Errorf("ForwardingRule %s is not deleted/error to get: %s", k.Name, err)
+			}
+		} else {
+			resources = append(resources, k)
+		}
+	}
+
+	switch protocol {
+	case HttpProtocol:
+		for k := range g.TargetHTTPProxy {
+			_, err := c.TargetHttpProxies().Get(ctx, &k)
+			if err != nil {
+				if err.(*googleapi.Error) == nil || err.(*googleapi.Error).Code != http.StatusNotFound {
+					return fmt.Errorf("TargetHTTPProxy %s is not deleted/error to get: %s", k.Name, err)
+				}
+			} else {
+				resources = append(resources, k)
+			}
+		}
+	case HttpsProtocol:
+		for k := range g.TargetHTTPSProxy {
+			_, err := c.TargetHttpsProxies().Get(ctx, &k)
+			if err != nil {
+				if err.(*googleapi.Error) == nil || err.(*googleapi.Error).Code != http.StatusNotFound {
+					return fmt.Errorf("TargetHTTPSProxy %s is not deleted/error to get: %s", k.Name, err)
+				}
+			} else {
+				resources = append(resources, k)
+			}
+		}
+	default:
+		return fmt.Errorf("invalid protocol %q", protocol)
+	}
+
+	if len(resources) != 0 {
+		var s []string
+		for _, r := range resources {
+			s = append(s, r.String())
+		}
+		return fmt.Errorf("resources still exist (%s)", strings.Join(s, ", "))
+	}
+
+	return nil
+}
+
+// getForwardingRuleProtocol returns the protocol for given forwarding rule.
+func getForwardingRuleProtocol(forwardingRule *compute.ForwardingRule) (Protocol, error) {
+	resID, err := cloud.ParseResourceURL(forwardingRule.Target)
+	if err != nil {
+		return "", fmt.Errorf("error parsing Target (%q): %v", forwardingRule.Target, err)
+	}
+	switch resID.Resource {
+	case targetHTTPProxyResource:
+		return HttpProtocol, nil
+	case targetHTTPSProxyResource:
+		return HttpsProtocol, nil
+	default:
+		return "", fmt.Errorf("unhandled resource %q", resID.Resource)
+	}
+}
+
+// CheckNEGDeletion checks that all NEGs associated with the GCLB have been deleted
 func (g *GCLB) CheckNEGDeletion(ctx context.Context, c cloud.Cloud, options *GCLBDeleteOptions) error {
 	var resources []meta.Key
 
@@ -318,6 +413,7 @@ func GCLBForVIP(ctx context.Context, c cloud.Cloud, params *GCLBForVIPParams) (*
 		}
 	}
 
+	// Return immediately if there are no forwarding rules exist.
 	if len(gfrs) == 0 {
 		klog.Warningf("No global forwarding rules found, can't get all GCLB resources")
 		return gclb, nil
@@ -346,7 +442,7 @@ func GCLBForVIP(ctx context.Context, c cloud.Cloud, params *GCLBForVIPParams) (*
 			return nil, err
 		}
 		switch resID.Resource {
-		case "targetHttpProxies":
+		case targetHTTPProxyResource:
 			p, err := c.TargetHttpProxies().Get(ctx, resID.Key)
 			if err != nil {
 				klog.Warningf("Error getting TargetHttpProxy %s: %v", resID.Key, err)
@@ -369,7 +465,7 @@ func GCLBForVIP(ctx context.Context, c cloud.Cloud, params *GCLBForVIPParams) (*
 				klog.Warningf("Error targetHttpProxy references are not the same (%s != %s)", *urlMapKey, *urlMapResID.Key)
 				return nil, fmt.Errorf("targetHttpProxy references are not the same: %+v != %+v", *urlMapKey, *urlMapResID.Key)
 			}
-		case "targetHttpsProxies":
+		case targetHTTPSProxyResource:
 			p, err := c.TargetHttpsProxies().Get(ctx, resID.Key)
 			if err != nil {
 				klog.Warningf("Error getting targetHttpsProxy (%s): %v", resID.Key, err)

pkg/fuzz/helpers.go

@@ -230,6 +230,12 @@ func (i *IngressBuilder) Path(host, path, service string, port intstr.IntOrStrin
 	return &h.HTTP.Paths[len(h.HTTP.Paths)-1]
 }
 
+// SetTLS sets TLS certs to given list.
+func (i *IngressBuilder) SetTLS(tlsCerts []v1beta1.IngressTLS) *IngressBuilder {
+	i.ing.Spec.TLS = tlsCerts
+	return i
+}
+
 // AddTLS adds a TLS secret reference.
 func (i *IngressBuilder) AddTLS(hosts []string, secretName string) *IngressBuilder {
 	i.ing.Spec.TLS = append(i.ing.Spec.TLS, v1beta1.IngressTLS{

pkg/fuzz/whitebox/num_forwarding_rules.go

@@ -36,18 +36,18 @@ func (t *numForwardingRulesTest) Name() string {
 
 // Test implements WhiteboxTest.
 func (t *numForwardingRulesTest) Test(ing *v1beta1.Ingress, gclb *fuzz.GCLB) error {
-	expectedForwardingRules := 1
-	if len(ing.Spec.TLS) > 0 {
-		expectedForwardingRules = 2
-	}
+	expectedForwardingRules := 0
 
 	an := annotations.FromIngress(ing)
-	if an.UseNamedTLS() != "" {
-		expectedForwardingRules = 2
+	if an.AllowHTTP() {
+		expectedForwardingRules += 1
+	}
+	if len(ing.Spec.TLS) > 0 || an.UseNamedTLS() != "" {
+		expectedForwardingRules += 1
 	}
 
 	if len(gclb.ForwardingRule) != expectedForwardingRules {
-		return fmt.Errorf("Expected %d ForwardingRule's but got %d", expectedForwardingRules, len(gclb.ForwardingRule))
+		return fmt.Errorf("expected %d ForwardingRule's but got %d", expectedForwardingRules, len(gclb.ForwardingRule))
 	}
 
 	return nil

pkg/fuzz/whitebox/num_target_proxies.go

@@ -0,0 +1,57 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package whitebox
+
+import (
+	"fmt"
+
+	"k8s.io/api/networking/v1beta1"
+	"k8s.io/ingress-gce/pkg/annotations"
+	"k8s.io/ingress-gce/pkg/fuzz"
+)
+
+// Implements a whitebox test to check that the GCLB has the expected number of TargetHTTPSProxy's.
+type numTargetProxiesTest struct {
+}
+
+// Name implements WhiteboxTest.
+func (t *numTargetProxiesTest) Name() string {
+	return "NumTargetProxiesTest"
+
+}
+
+// Test implements WhiteboxTest.
+func (t *numTargetProxiesTest) Test(ing *v1beta1.Ingress, gclb *fuzz.GCLB) error {
+	expectedHTTPTargetProxies, expectedHTTPSTargetProxies := 0, 0
+
+	an := annotations.FromIngress(ing)
+	if an.AllowHTTP() {
+		expectedHTTPTargetProxies = 1
+	}
+	if len(ing.Spec.TLS) > 0 || an.UseNamedTLS() != "" {
+		expectedHTTPSTargetProxies = 1
+	}
+
+	if l := len(gclb.TargetHTTPProxy); l != expectedHTTPTargetProxies {
+		return fmt.Errorf("expected %d TargetHTTPProxy's but got %d", expectedHTTPTargetProxies, l)
+	}
+	if l := len(gclb.TargetHTTPSProxy); l != expectedHTTPSTargetProxies {
+		return fmt.Errorf("expected %d TargetHTTPSProxy's but got %d", expectedHTTPSTargetProxies, l)
+	}
+
+	return nil
+}

pkg/fuzz/whitebox/whitebox_tests.go

@@ -22,4 +22,5 @@ import "k8s.io/ingress-gce/pkg/fuzz"
 var AllTests = []fuzz.WhiteboxTest{
 	&numBackendServicesTest{},
 	&numForwardingRulesTest{},
+	&numTargetProxiesTest{},
 }