From 3d188a02cfa2ebf9e9764ccfc03a7c8f6b0f7d74 Mon Sep 17 00:00:00 2001 From: Satish Matti Date: Mon, 14 Oct 2019 13:27:15 -0700 Subject: [PATCH] Delete unused GCE loadbalancer resources on updating ingress spec --- pkg/annotations/ingress.go | 21 +- pkg/flags/flags.go | 2 + pkg/loadbalancers/addresses.go | 21 +- pkg/loadbalancers/l7.go | 261 +++++++++++++++++------- pkg/loadbalancers/l7s_test.go | 45 ++-- pkg/loadbalancers/loadbalancers_test.go | 140 ++++++++++++- 6 files changed, 389 insertions(+), 101 deletions(-) diff --git a/pkg/annotations/ingress.go b/pkg/annotations/ingress.go index d3ec1168ce..dd674dfce4 100644 --- a/pkg/annotations/ingress.go +++ b/pkg/annotations/ingress.go @@ -42,7 +42,7 @@ const ( StaticIPNameKey = "kubernetes.io/ingress.global-static-ip-name" // PreSharedCertKey represents the specific pre-shared SSL - // certicate for the Ingress controller to use. The controller *does not* + // certificate for the Ingress controller to use. The controller *does not* // manage this certificate, it is the users responsibility to create/delete it. // In GCP, the Ingress controller assigns the SSL certificate with this name // to the target proxies of the Ingress. @@ -77,6 +77,25 @@ const ( // - annotations: // networking.gke.io/v1beta1.FrontendConfig: 'my-frontendconfig' FrontendConfigKey = "networking.gke.io/v1beta1.FrontendConfig" + + // UrlMapKey is the annotation key used by controller to record GCP URL map. + UrlMapKey = StatusPrefix + "/url-map" + // HttpForwardingRuleKey is the annotation key used by controller to record + // GCP http forwarding rule. + HttpForwardingRuleKey = StatusPrefix + "/forwarding-rule" + // HttpsForwardingRuleKey is the annotation key used by controller to record + // GCP https forwarding rule. + HttpsForwardingRuleKey = StatusPrefix + "/https-forwarding-rule" + // TargetHttpProxyKey is the annotation key used by controller to record + // GCP target http proxy. + TargetHttpProxyKey = StatusPrefix + "/target-proxy" + // TargetHttpsProxyKey is the annotation key used by controller to record + // GCP target https proxy. + TargetHttpsProxyKey = StatusPrefix + "/https-target-proxy" + // SSLCertKey is the annotation key used by controller to record GCP ssl cert. + SSLCertKey = StatusPrefix + "/ssl-cert" + // StaticIPKey is the annotation key used by controller to record GCP static ip. + StaticIPKey = StatusPrefix + "/static-ip" ) // Ingress represents ingress annotations. diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 4587ff46ec..6c33fe8629 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -88,6 +88,7 @@ var ( ASMConfigMapBasedConfigNamespace string ASMConfigMapBasedConfigCMName string EnableNonGCPMode bool + EnableDeleteUnusedFrontends bool LeaderElection LeaderElectionConfiguration }{} @@ -206,6 +207,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 flag.StringVar(&F.ASMConfigMapBasedConfigNamespace, "asm-configmap-based-config-namespace", "kube-system,istio-system", "ASM Configmap based config: configmap namespace") flag.StringVar(&F.ASMConfigMapBasedConfigCMName, "asm-configmap-based-config-cmname", "ingress-controller-asm-cm-config", "ASM Configmap based config: configmap name") flag.BoolVar(&F.EnableNonGCPMode, "enable-non-gcp-mode", false, "Set to true when running on a non-GCP cluster.") + flag.BoolVar(&F.EnableDeleteUnusedFrontends, "enable-delete-unused-frontends", false, "Enable deleting unused gce frontend resources.") } type RateLimitSpecs struct { diff --git a/pkg/loadbalancers/addresses.go b/pkg/loadbalancers/addresses.go index 61a167076d..f7c943d67f 100644 --- a/pkg/loadbalancers/addresses.go +++ b/pkg/loadbalancers/addresses.go @@ -21,6 +21,8 @@ import ( "net/http" "google.golang.org/api/compute/v1" + "k8s.io/ingress-gce/pkg/annotations" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" "k8s.io/klog" @@ -31,26 +33,33 @@ func (l *L7) checkStaticIP() (err error) { if l.fw == nil || l.fw.IPAddress == "" { return fmt.Errorf("will not create static IP without a forwarding rule") } + managedStaticIPName := l.namer.ForwardingRule(namer.HTTPProtocol) // Don't manage staticIPs if the user has specified an IP. if address, manageStaticIP := l.getEffectiveIP(); !manageStaticIP { klog.V(3).Infof("Not managing user specified static IP %v", address) + if flags.F.EnableDeleteUnusedFrontends { + // Delete ingress controller managed static ip if exists. + if ip, ok := l.ingress.Annotations[annotations.StaticIPKey]; ok && ip == managedStaticIPName { + return l.deleteStaticIP() + } + } return nil } - staticIPName := l.namer.ForwardingRule(namer.HTTPProtocol) - ip, _ := l.cloud.GetGlobalAddress(staticIPName) + + ip, _ := l.cloud.GetGlobalAddress(managedStaticIPName) if ip == nil { - klog.V(3).Infof("Creating static ip %v", staticIPName) - err = l.cloud.ReserveGlobalAddress(&compute.Address{Name: staticIPName, Address: l.fw.IPAddress}) + klog.V(3).Infof("Creating static ip %v", managedStaticIPName) + err = l.cloud.ReserveGlobalAddress(&compute.Address{Name: managedStaticIPName, Address: l.fw.IPAddress}) if err != nil { if utils.IsHTTPErrorCode(err, http.StatusConflict) || utils.IsHTTPErrorCode(err, http.StatusBadRequest) { klog.V(3).Infof("IP %v(%v) is already reserved, assuming it is OK to use.", - l.fw.IPAddress, staticIPName) + l.fw.IPAddress, managedStaticIPName) return nil } return err } - ip, err = l.cloud.GetGlobalAddress(staticIPName) + ip, err = l.cloud.GetGlobalAddress(managedStaticIPName) if err != nil { return err } diff --git a/pkg/loadbalancers/l7.go b/pkg/loadbalancers/l7.go index f30d5349fb..7c6af4aac3 100644 --- a/pkg/loadbalancers/l7.go +++ b/pkg/loadbalancers/l7.go @@ -171,6 +171,11 @@ func (l *L7) edgeHop() error { if err := l.edgeHopHttp(); err != nil { return err } + } else if flags.F.EnableDeleteUnusedFrontends && requireDeleteFrontend(l.ingress, namer.HTTPProtocol) { + if err := l.deleteHttp(features.VersionsFromIngress(&l.ingress)); err != nil { + return err + } + klog.V(2).Infof("Successfully deleted unused HTTP frontend resources for load-balancer %s", l) } // Defer promoting an ephemeral to a static IP until it's really needed. if l.runtimeInfo.AllowHTTP && sslConfigured { @@ -185,6 +190,11 @@ func (l *L7) edgeHop() error { if err := l.edgeHopHttps(); err != nil { return err } + } else if flags.F.EnableDeleteUnusedFrontends && requireDeleteFrontend(l.ingress, namer.HTTPSProtocol) { + if err := l.deleteHttps(features.VersionsFromIngress(&l.ingress)); err != nil { + return err + } + klog.V(2).Infof("Successfully deleted unused HTTPS frontend resources for load-balancer %s", l) } if !willConfigureFrontend { @@ -216,6 +226,32 @@ func (l *L7) edgeHopHttps() error { return l.checkHttpsForwardingRule() } +// requireDeleteFrontend returns true if gce loadbalancer resources needs to deleted for given protocol. +func requireDeleteFrontend(ing v1beta1.Ingress, protocol namer.NamerProtocol) bool { + var keys []string + switch protocol { + case namer.HTTPSProtocol: + keys = append(keys, []string{ + annotations.HttpsForwardingRuleKey, + annotations.TargetHttpsProxyKey, + }...) + case namer.HTTPProtocol: + keys = append(keys, []string{ + annotations.HttpForwardingRuleKey, + annotations.TargetHttpProxyKey, + }...) + default: + klog.Errorf("Unexpected frontend resource protocol %v", protocol) + } + + for _, key := range keys { + if _, exists := ing.Annotations[key]; exists { + return true + } + } + return false +} + // GetIP returns the ip associated with the forwarding rule for this l7. func (l *L7) GetIP() string { if l.fw != nil { @@ -227,98 +263,189 @@ func (l *L7) GetIP() string { return "" } -// Cleanup deletes resources specific to this l7 in the right order. -// forwarding rule -> target proxy -> url map -// This leaves backends and health checks, which are shared across loadbalancers. -func (l *L7) Cleanup(versions *features.ResourceVersions) error { - var key *meta.Key - var err error - - fwName := l.namer.ForwardingRule(namer.HTTPProtocol) - klog.V(2).Infof("Deleting global forwarding rule %v", fwName) - if key, err = l.CreateKey(fwName); err != nil { +// deleteForwardingRule deletes forwarding rule for given protocol. +func (l *L7) deleteForwardingRule(versions *features.ResourceVersions, protocol namer.NamerProtocol) error { + frName := l.namer.ForwardingRule(protocol) + klog.V(2).Infof("Deleting global forwarding rule %v", frName) + key, err := l.CreateKey(frName) + if err != nil { return err } if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, versions.ForwardingRule)); err != nil { return err } + return nil +} - fwsName := l.namer.ForwardingRule(namer.HTTPSProtocol) - klog.V(2).Infof("Deleting global forwarding rule %v", fwsName) - if key, err = l.CreateKey(fwsName); err != nil { - return err - } - if err := utils.IgnoreHTTPNotFound(composite.DeleteForwardingRule(l.cloud, key, versions.ForwardingRule)); err != nil { +// deleteTargetProxy deletes target proxy for given protocol. +func (l *L7) deleteTargetProxy(versions *features.ResourceVersions, protocol namer.NamerProtocol) error { + tpName := l.namer.TargetProxy(protocol) + klog.V(2).Infof("Deleting target %v proxy %v", protocol, tpName) + key, err := l.CreateKey(tpName) + if err != nil { return err } - - ip, err := l.cloud.GetGlobalAddress(fwName) - if ip != nil && utils.IgnoreHTTPNotFound(err) == nil { - klog.V(2).Infof("Deleting static IP %v(%v)", ip.Name, ip.Address) - if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalAddress(ip.Name)); err != nil { + switch protocol { + case namer.HTTPProtocol: + if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpProxy(l.cloud, key, versions.TargetHttpProxy)); err != nil { return err } + case namer.HTTPSProtocol: + if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpsProxy(l.cloud, key, versions.TargetHttpsProxy)); err != nil { + return err + } + default: + return fmt.Errorf("unexpected frontend resource protocol: %v", protocol) } + return nil +} - tpName := l.namer.TargetProxy(namer.HTTPProtocol) - klog.V(2).Infof("Deleting target http proxy %v", tpName) - if key, err = l.CreateKey(tpName); err != nil { - return err - } - if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpProxy(l.cloud, key, versions.TargetHttpProxy)); err != nil { +// deleteHttp deletes http forwarding rule and target http proxy. +func (l *L7) deleteHttp(versions *features.ResourceVersions) error { + // Delete http forwarding rule. + if err := l.deleteForwardingRule(versions, namer.HTTPProtocol); err != nil { return err } + // Delete target http proxy. + return l.deleteTargetProxy(versions, namer.HTTPProtocol) +} - tpsName := l.namer.TargetProxy(namer.HTTPSProtocol) - klog.V(2).Infof("Deleting target https proxy %v", tpsName) - if key, err = l.CreateKey(tpsName); err != nil { - return err - } - if err := utils.IgnoreHTTPNotFound(composite.DeleteTargetHttpsProxy(l.cloud, key, versions.TargetHttpsProxy)); err != nil { +// deleteHttps deletes https forwarding rule, target https proxy and ingress controller +// managed ssl certificates. +func (l *L7) deleteHttps(versions *features.ResourceVersions) error { + // Delete https forwarding rule. + if err := l.deleteForwardingRule(versions, namer.HTTPSProtocol); err != nil { return err } - - // Delete the SSL cert if it is from a secret, not referencing a pre-created GCE cert or managed certificates. + // Get list of ssl certificates owned by this load-balancer that needs to be deleted. + // We are using https target proxy to list legacy certs, so this list needs to be + // populated before deleting https target proxy. secretsSslCerts, err := l.getIngressManagedSslCerts() if err != nil { return err } + // Delete target https proxy. + if err := l.deleteTargetProxy(versions, namer.HTTPSProtocol); err != nil { + return err + } + // Delete ingress managed ssl certificates those created from a secret, + // not referencing a pre-created GCE cert or managed certificates. + return l.deleteSSLCertificates(secretsSslCerts, versions) +} - if len(secretsSslCerts) != 0 { - var certErr error - for _, cert := range secretsSslCerts { - klog.V(2).Infof("Deleting sslcert %s", cert.Name) - if key, err = l.CreateKey(cert.Name); err != nil { - return err - } - if err := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, versions.SslCertificate)); err != nil { - klog.Errorf("Old cert delete failed - %v", err) - certErr = err - } +// deleteSSLCertificates deletes given ssl certificates. +func (l *L7) deleteSSLCertificates(sslCertificates []*composite.SslCertificate, versions *features.ResourceVersions) error { + if len(sslCertificates) == 0 { + return nil + } + var certErr error + for _, cert := range sslCertificates { + klog.V(2).Infof("Deleting sslcert %s", cert.Name) + key, err := l.CreateKey(cert.Name) + if err != nil { + return err } - l.sslCerts = nil - if certErr != nil { - return certErr + if err := utils.IgnoreHTTPNotFound(composite.DeleteSslCertificate(l.cloud, key, versions.SslCertificate)); err != nil { + klog.Errorf("Old cert delete failed - %v", err) + certErr = err } } + l.sslCerts = nil + return certErr +} +// deleteStaticIP deletes ingress managed static ip. +func (l *L7) deleteStaticIP() error { + frName := l.namer.ForwardingRule(namer.HTTPProtocol) + ip, err := l.cloud.GetGlobalAddress(frName) + if ip != nil && utils.IgnoreHTTPNotFound(err) == nil { + klog.V(2).Infof("Deleting static IP %v(%v)", ip.Name, ip.Address) + if err := utils.IgnoreHTTPNotFound(l.cloud.DeleteGlobalAddress(ip.Name)); err != nil { + return err + } + } + return nil +} + +// Cleanup deletes resources specific to this l7 in the right order. +// forwarding rule -> target proxy -> url map +// This leaves backends and health checks, which are shared across loadbalancers. +func (l *L7) Cleanup(versions *features.ResourceVersions) error { + var err error + // Delete http frontend resources. + if err := l.deleteHttp(versions); err != nil { + return err + } + // Delete static ip. + if err := l.deleteStaticIP(); err != nil { + return err + } + // Delete https frontend resources. + if err := l.deleteHttps(versions); err != nil { + return err + } + // Delete URL map. umName := l.namer.UrlMap() klog.V(2).Infof("Deleting URL Map %v", umName) - if key, err = l.CreateKey(umName); err != nil { + key, err := l.CreateKey(umName) + if err != nil { return err } if err := utils.IgnoreHTTPNotFound(composite.DeleteUrlMap(l.cloud, key, versions.UrlMap)); err != nil { return err } - return nil } -// GetLBAnnotations returns the annotations of an l7. This includes it's current status. -func GetLBAnnotations(l7 *L7, existing map[string]string, backendSyncer backends.Syncer) (map[string]string, error) { +func (l *L7) getFrontendAnnotations(existing map[string]string) map[string]string { if existing == nil { existing = map[string]string{} } + + var certs []string + for _, cert := range l.sslCerts { + certs = append(certs, cert.Name) + } + + existing[annotations.UrlMapKey] = l.um.Name + // Forwarding rule and target proxy might not exist if allowHTTP == false + if l.fw != nil { + existing[annotations.HttpForwardingRuleKey] = l.fw.Name + } else { + delete(existing, annotations.HttpForwardingRuleKey) + } + if l.tp != nil { + existing[annotations.TargetHttpProxyKey] = l.tp.Name + } else { + delete(existing, annotations.TargetHttpProxyKey) + } + // HTTPs resources might not exist if TLS == nil + if l.fws != nil { + existing[annotations.HttpsForwardingRuleKey] = l.fws.Name + } else { + delete(existing, annotations.HttpsForwardingRuleKey) + } + if l.tps != nil { + existing[annotations.TargetHttpsProxyKey] = l.tps.Name + } else { + delete(existing, annotations.TargetHttpsProxyKey) + } + // Note that ingress IP annotation is not deleted when user disables one of http/https. + // This is because the promoted static IP is retained for use and will be deleted only + // when load-balancer is deleted or user specifies a different IP. + if l.ip != nil { + existing[annotations.StaticIPKey] = l.ip.Name + } + if len(certs) > 0 { + existing[annotations.SSLCertKey] = strings.Join(certs, ",") + } else { + delete(existing, annotations.SSLCertKey) + } + return existing +} + +// GetLBAnnotations returns the annotations of an l7. This includes it's current status. +func GetLBAnnotations(l7 *L7, existing map[string]string, backendSyncer backends.Syncer) (map[string]string, error) { backends, err := getBackendNames(l7.um) if err != nil { return nil, err @@ -338,32 +465,8 @@ func GetLBAnnotations(l7 *L7, existing map[string]string, backendSyncer backends if err == nil { jsonBackendState = string(b) } - certs := []string{} - for _, cert := range l7.sslCerts { - certs = append(certs, cert.Name) - } - - existing[fmt.Sprintf("%v/url-map", annotations.StatusPrefix)] = l7.um.Name - // Forwarding rule and target proxy might not exist if allowHTTP == false - if l7.fw != nil { - existing[fmt.Sprintf("%v/forwarding-rule", annotations.StatusPrefix)] = l7.fw.Name - } - if l7.tp != nil { - existing[fmt.Sprintf("%v/target-proxy", annotations.StatusPrefix)] = l7.tp.Name - } - // HTTPs resources might not exist if TLS == nil - if l7.fws != nil { - existing[fmt.Sprintf("%v/https-forwarding-rule", annotations.StatusPrefix)] = l7.fws.Name - } - if l7.tps != nil { - existing[fmt.Sprintf("%v/https-target-proxy", annotations.StatusPrefix)] = l7.tps.Name - } - if l7.ip != nil { - existing[fmt.Sprintf("%v/static-ip", annotations.StatusPrefix)] = l7.ip.Name - } - if len(certs) > 0 { - existing[fmt.Sprintf("%v/ssl-cert", annotations.StatusPrefix)] = strings.Join(certs, ",") - } + // Update annotations for frontend resources. + existing = l7.getFrontendAnnotations(existing) // TODO: We really want to know *when* a backend flipped states. existing[fmt.Sprintf("%v/backends", annotations.StatusPrefix)] = jsonBackendState return existing, nil diff --git a/pkg/loadbalancers/l7s_test.go b/pkg/loadbalancers/l7s_test.go index c7f04fb111..54e78c23be 100644 --- a/pkg/loadbalancers/l7s_test.go +++ b/pkg/loadbalancers/l7s_test.go @@ -339,33 +339,50 @@ func removeFakeLoadBalancer(cloud *gce.Cloud, namer namer_util.IngressFrontendNa } func checkFakeLoadBalancer(cloud *gce.Cloud, namer namer_util.IngressFrontendNamer, versions *features.ResourceVersions, scope meta.KeyType, expectPresent bool) error { - var err error - key, _ := composite.CreateKey(cloud, namer.ForwardingRule(namer_util.HTTPProtocol), scope) - - _, err = composite.GetForwardingRule(cloud, key, versions.ForwardingRule) + if err := checkFakeLoadBalancerWithProtocol(cloud, namer, versions, scope, expectPresent, namer_util.HTTPProtocol); err != nil { + return err + } + key, _ := composite.CreateKey(cloud, namer.UrlMap(), scope) + _, err := composite.GetUrlMap(cloud, key, versions.UrlMap) if expectPresent && err != nil { return err } - if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { - return fmt.Errorf("expect GlobalForwardingRule %q to not present: %v", key, err) + if !expectPresent && (err == nil || err.(*googleapi.Error).Code != http.StatusNotFound) { + return fmt.Errorf("expect URLMap %q to not present: %v", key, err) } + return nil +} - key.Name = namer.TargetProxy(namer_util.HTTPProtocol) - _, err = composite.GetTargetHttpProxy(cloud, key, versions.TargetHttpProxy) +func checkBothFakeLoadBalancers(cloud *gce.Cloud, namer namer_util.IngressFrontendNamer, versions *features.ResourceVersions, scope meta.KeyType, expectHttp, expectHttps bool) error { + if err := checkFakeLoadBalancerWithProtocol(cloud, namer, versions, scope, expectHttp, namer_util.HTTPProtocol); err != nil { + return err + } + return checkFakeLoadBalancerWithProtocol(cloud, namer, versions, scope, expectHttps, namer_util.HTTPSProtocol) +} + +func checkFakeLoadBalancerWithProtocol(cloud *gce.Cloud, namer namer_util.IngressFrontendNamer, versions *features.ResourceVersions, scope meta.KeyType, expectPresent bool, protocol namer_util.NamerProtocol) error { + var err error + key, _ := composite.CreateKey(cloud, namer.ForwardingRule(protocol), scope) + + _, err = composite.GetForwardingRule(cloud, key, versions.ForwardingRule) if expectPresent && err != nil { return err } - if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { - return fmt.Errorf("expect TargetHTTPProxy %q to not present: %v", key, err) + if !expectPresent && (err == nil || err.(*googleapi.Error).Code != http.StatusNotFound) { + return fmt.Errorf("expect %s GlobalForwardingRule %q to not present: %v", protocol, key, err) } - key.Name = namer.UrlMap() - _, err = composite.GetUrlMap(cloud, key, versions.UrlMap) + key.Name = namer.TargetProxy(protocol) + if protocol == namer_util.HTTPProtocol { + _, err = composite.GetTargetHttpProxy(cloud, key, versions.TargetHttpProxy) + } else { + _, err = composite.GetTargetHttpsProxy(cloud, key, versions.TargetHttpsProxy) + } if expectPresent && err != nil { return err } - if !expectPresent && err.(*googleapi.Error).Code != http.StatusNotFound { - return fmt.Errorf("expect URLMap %q to not present: %v", key, err) + if !expectPresent && (err == nil || err.(*googleapi.Error).Code != http.StatusNotFound) { + return fmt.Errorf("expect Target%sProxy %q to not present: %v", protocol, key, err) } return nil } diff --git a/pkg/loadbalancers/loadbalancers_test.go b/pkg/loadbalancers/loadbalancers_test.go index 9c530303c8..ff1c64036c 100644 --- a/pkg/loadbalancers/loadbalancers_test.go +++ b/pkg/loadbalancers/loadbalancers_test.go @@ -27,6 +27,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" + "github.com/google/go-cmp/cmp" "google.golang.org/api/compute/v1" "google.golang.org/api/googleapi" "k8s.io/api/networking/v1beta1" @@ -35,6 +36,7 @@ import ( "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/events" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/instances" "k8s.io/ingress-gce/pkg/loadbalancers/features" "k8s.io/ingress-gce/pkg/utils" @@ -866,6 +868,10 @@ func verifyCertAndProxyLink(expectCerts map[string]string, expectCertsProxy map[ } tps, err := composite.GetTargetHttpsProxy(j.fakeGCE, key, defaultVersion) if err != nil { + // Return immediately if expected certs is an empty map. + if len(expectCertsProxy) == 0 && err.(*googleapi.Error).Code == http.StatusNotFound { + return + } t.Fatalf("expected https proxy to exist: %v, err: %v", j.feNamer.TargetProxy(namer_util.HTTPSProtocol), err) } if len(tps.SslCertificates) != len(expectCertsProxy) { @@ -1076,7 +1082,7 @@ func TestNameParsing(t *testing.T) { namer := namer_util.NewNamer(clusterName, firewallName) fullName := namer.ForwardingRule(namer.LoadBalancer("testlb"), namer_util.HTTPProtocol) annotationsMap := map[string]string{ - fmt.Sprintf("%v/forwarding-rule", annotations.StatusPrefix): fullName, + fmt.Sprintf(annotations.HttpForwardingRuleKey): fullName, } components := namer.ParseName(GCEResourceName(annotationsMap, "forwarding-rule")) t.Logf("components = %+v", components) @@ -1476,3 +1482,135 @@ func TestSecretBasedToPreSharedCertUpdateWithErrors(t *testing.T) { } verifyCertAndProxyLink(expectCerts, expectCerts, j, t) } + +// TestResourceDeletionWithProtocol asserts that unused resources are cleaned up +// on updating ingress configuration to disable http/https traffic. +func TestResourceDeletionWithProtocol(t *testing.T) { + // TODO(smatti): Add flag saver to capture current value and reset back. + flags.F.EnableDeleteUnusedFrontends = true + j := newTestJig(t) + + gceUrlMap := utils.NewGCEURLMap() + gceUrlMap.DefaultBackend = &utils.ServicePort{NodePort: 31234, BackendNamer: j.namer} + gceUrlMap.PutPathRulesForHost("bar.example.com", []utils.PathRule{{Path: "/bar", Backend: utils.ServicePort{NodePort: 30000, BackendNamer: j.namer}}}) + ing := newIngress() + feNamer := namer_util.NewFrontendNamerFactory(j.namer).Namer(ing) + versions := features.GAResourceVersions + certName1 := feNamer.SSLCertName(GetCertHash("cert1")) + + for _, tc := range []struct { + desc string + disableHTTP bool + disableHTTPS bool + }{ + {"both enabled", false, false}, + {"http only", false, true}, + {"https only", true, false}, + {"both disabled", true, true}, + } { + t.Run(tc.desc, func(t *testing.T) { + lbInfo := &L7RuntimeInfo{ + AllowHTTP: true, + TLS: []*TLSCerts{ + createCert("key1", "cert1", "secret1"), + }, + UrlMap: gceUrlMap, + Ingress: ing, + } + lb, err := j.pool.Ensure(lbInfo) + if err != nil { + t.Fatalf("pool.Ensure(%+v) = %v, want nil", lbInfo, err) + } + // Update ingress annotations + ing.Annotations = lb.getFrontendAnnotations(make(map[string]string)) + verifyLBAnnotations(t, lb, ing.Annotations) + + expectCerts := map[string]string{certName1: lbInfo.TLS[0].Cert} + verifyCertAndProxyLink(expectCerts, expectCerts, j, t) + if err := checkBothFakeLoadBalancers(j.fakeGCE, feNamer, versions, defaultScope, true, true); err != nil { + t.Errorf("checkFakeLoadBalancer(..., true, true) = %v, want nil for case %q and key %q", err, tc.desc, common.NamespacedName(ing)) + } + + expectHttp, expectHttps := true, true + if tc.disableHTTP { + lbInfo.AllowHTTP = false + expectHttp = false + } + if tc.disableHTTPS { + lbInfo.TLS = nil + expectHttps = false + delete(expectCerts, certName1) + } + + if lb, err = j.pool.Ensure(lbInfo); err != nil { + t.Fatalf("pool.Ensure(%+v) = %v, want nil", lbInfo, err) + } + // Update ingress annotations + ing.Annotations = lb.getFrontendAnnotations(make(map[string]string)) + verifyLBAnnotations(t, lb, ing.Annotations) + + verifyCertAndProxyLink(expectCerts, expectCerts, j, t) + if err := checkBothFakeLoadBalancers(j.fakeGCE, feNamer, versions, defaultScope, expectHttp, expectHttps); err != nil { + t.Errorf("checkFakeLoadBalancer(..., %t, %t) = %v, want nil for case %q and key %q", expectHttp, expectHttps, err, tc.desc, common.NamespacedName(ing)) + } + }) + } +} + +// verifyLBAnnotations asserts that ingress annotations updated correctly. +func verifyLBAnnotations(t *testing.T, l7 *L7, ingAnnotations map[string]string) { + var l7Certs []string + for _, cert := range l7.sslCerts { + l7Certs = append(l7Certs, cert.Name) + } + fw, exists := ingAnnotations[annotations.HttpForwardingRuleKey] + if l7.fw != nil { + if !exists { + t.Errorf("Expected http forwarding rule annotation to exist") + } else if diff := cmp.Diff(l7.fw.Name, fw); diff != "" { + t.Errorf("Got diff for http forwarding rule (-want +got):\n%s", diff) + } + } else if exists { + t.Errorf("Expected http forwarding rule annotation to not exist") + } + tp, exists := ingAnnotations[annotations.TargetHttpProxyKey] + if l7.tp != nil { + if !exists { + t.Errorf("Expected target http proxy annotation to exist") + } else if diff := cmp.Diff(l7.tp.Name, tp); diff != "" { + t.Errorf("Got diff for target http proxy (-want +got):\n%s", diff) + } + } else if exists { + t.Errorf("Expected target http proxy annotation to not exist") + } + fws, exists := ingAnnotations[annotations.HttpsForwardingRuleKey] + if l7.fws != nil { + if !exists { + t.Errorf("Expected https forwarding rule annotation to exist") + } else if diff := cmp.Diff(l7.fws.Name, fws); diff != "" { + t.Errorf("Got diff for https forwarding rule (-want +got):\n%s", diff) + } + } else if exists { + t.Errorf("Expected https forwarding rule annotation to not exist") + } + tps, exists := ingAnnotations[annotations.TargetHttpsProxyKey] + if l7.tps != nil { + if !exists { + t.Errorf("Expected target https proxy annotation to exist") + } else if diff := cmp.Diff(l7.tps.Name, tps); diff != "" { + t.Errorf("Got diff for target https proxy (-want +got):\n%s", diff) + } + } else if exists { + t.Errorf("Expected target https proxy annotation to not exist") + } + certs, exists := ingAnnotations[annotations.SSLCertKey] + if len(l7Certs) > 0 { + if !exists { + t.Errorf("Expected ssl cert annotation to exist") + } else if diff := cmp.Diff(strings.Join(l7Certs, ","), certs); diff != "" { + t.Errorf("Got diff for ssl certs (-want +got):\n%s", diff) + } + } else if exists { + t.Errorf("Expected ssl cert annotation to not exist") + } +}