From dec47ae4654e2393c851614058f48d91763cfd20 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Thu, 17 Oct 2019 10:37:00 -0700 Subject: [PATCH] Add e2e tests for frontend resource leak fix --- cmd/e2e-test/basic_test.go | 111 ++++++++++++++++++++++ pkg/e2e/fixtures.go | 2 +- pkg/e2e/helpers.go | 20 ++++ pkg/fuzz/gcp.go | 100 ++++++++++++++++++- pkg/fuzz/helpers.go | 6 ++ pkg/fuzz/whitebox/num_forwarding_rules.go | 14 +-- pkg/fuzz/whitebox/num_target_proxies.go | 57 +++++++++++ pkg/fuzz/whitebox/whitebox_tests.go | 1 + 8 files changed, 302 insertions(+), 9 deletions(-) create mode 100644 pkg/fuzz/whitebox/num_target_proxies.go diff --git a/cmd/e2e-test/basic_test.go b/cmd/e2e-test/basic_test.go index 115487ed3f..b52c431c53 100644 --- a/cmd/e2e-test/basic_test.go +++ b/cmd/e2e-test/basic_test.go @@ -230,3 +230,114 @@ func whiteboxTest(ing *v1beta1.Ingress, s *e2e.Sandbox, t *testing.T) *fuzz.GCLB } 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" + + for _, tc := range []struct { + desc string + disableHTTP bool + disableHTTPS bool + }{ + {"both enabled", false, false}, + {"http only", false, true}, + {"http only",true, false}, + {"both disabled",true, true}, + } { + tc := tc + desc := fmt.Sprintf("DisabelHTTP %t DisableHTTPS %t", tc.disableHTTP, tc.disableHTTPS) + Framework.RunWithSandbox(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-resource-deletion--%s", s.Namespace) + 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, "ingress-1", ""). + 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) + 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) + + 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) + } + + 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) + } + + // Special case in which we whitebox test just the number of forwarding rules. + if tc.disableHTTP && tc.disableHTTPS { + if gclb, err := fuzz.GCLBForVIP(context.Background(), Framework.Cloud, vip, fuzz.FeatureValidators(features.All)); err != nil { + t.Fatalf("GCLBForVIP(..., %q, _) = %v, want nil; error getting GCP resources for LB with IP", vip, err) + } else if l := len(gclb.ForwardingRule); l != 0 { + t.Fatalf("Expected 0 ForwardingRule's but got %d", l) + } + } else { + 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) + } + }) + } +} diff --git a/pkg/e2e/fixtures.go b/pkg/e2e/fixtures.go index 432b69483d..add924b7df 100644 --- a/pkg/e2e/fixtures.go +++ b/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 } diff --git a/pkg/e2e/helpers.go b/pkg/e2e/helpers.go index 526d53bd55..96e0a67f9e 100644 --- a/pkg/e2e/helpers.go +++ b/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) { diff --git a/pkg/fuzz/gcp.go b/pkg/fuzz/gcp.go index a1d57048d2..22cfdb37a6 100644 --- a/pkg/fuzz/gcp.go +++ b/pkg/fuzz/gcp.go @@ -39,8 +39,13 @@ import ( const ( NegResourceType = "networkEndpointGroup" IgResourceType = "instanceGroup" + HttpProtocol = Protocol("HTTP") + HttpsProtocol = Protocol("HTTPS") ) +// 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 +135,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 @@ -222,7 +233,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 "targetHttpProxies": + return HttpProtocol, nil + case "targetHttpsProxies": + 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 @@ -284,6 +377,11 @@ func GCLBForVIP(ctx context.Context, c cloud.Cloud, vip string, validators []Fea } } + // Return immediately if there are no forwarding rules exist. + if len(gfrs) == 0 { + return gclb, nil + } + var urlMapKey *meta.Key for _, gfr := range gfrs { frKey := meta.GlobalKey(gfr.Name) diff --git a/pkg/fuzz/helpers.go b/pkg/fuzz/helpers.go index b59c431972..4c06d9d30c 100644 --- a/pkg/fuzz/helpers.go +++ b/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{ diff --git a/pkg/fuzz/whitebox/num_forwarding_rules.go b/pkg/fuzz/whitebox/num_forwarding_rules.go index 2db167185d..f8bf3e7a6e 100644 --- a/pkg/fuzz/whitebox/num_forwarding_rules.go +++ b/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 diff --git a/pkg/fuzz/whitebox/num_target_proxies.go b/pkg/fuzz/whitebox/num_target_proxies.go new file mode 100644 index 0000000000..6e7b41af0b --- /dev/null +++ b/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 +} diff --git a/pkg/fuzz/whitebox/whitebox_tests.go b/pkg/fuzz/whitebox/whitebox_tests.go index 3e58b0d080..8d7c4942d8 100644 --- a/pkg/fuzz/whitebox/whitebox_tests.go +++ b/pkg/fuzz/whitebox/whitebox_tests.go @@ -22,4 +22,5 @@ import "k8s.io/ingress-gce/pkg/fuzz" var AllTests = []fuzz.WhiteboxTest{ &numBackendServicesTest{}, &numForwardingRulesTest{}, + &numTargetProxiesTest{}, }