From 44bdaa76cc7f65537e64250ec800e39b247192dd Mon Sep 17 00:00:00 2001
From: Ville Aikas <vaikas-google@users.noreply.github.com>
Date: Thu, 24 May 2018 09:49:08 -0700
Subject: [PATCH] Fix bind delete (#59)

* Update the deploying of function code to be separate from hte bind to demonstrate late binding

* Handle deletes more robustly.

* address CR comments

* Convert to grpc status and look at the code instead of string matching
---
 pkg/controller/bind/controller.go | 37 ++++++++++++++++++-------------
 pkg/sources/gcp_pubsub.go         | 18 +++++++++++++--
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/pkg/controller/bind/controller.go b/pkg/controller/bind/controller.go
index 815c19b7aa8..611673b80f6 100644
--- a/pkg/controller/bind/controller.go
+++ b/pkg/controller/bind/controller.go
@@ -293,24 +293,40 @@ func (c *Controller) syncHandler(key string) error {
 	// Don't mutate the informer's copy of our object.
 	bind = bind.DeepCopy()
 
+	// See if the binding has been deleted
+	accessor, err := meta.Accessor(bind)
+	if err != nil {
+		log.Fatalf("Failed to get metadata: %s", err)
+	}
+	deletionTimestamp := accessor.GetDeletionTimestamp()
+
 	// Find the Route that they want.
 	routeName := bind.Spec.Action.RouteName
 	route, err := c.routesLister.Routes(namespace).Get(routeName)
-	if err != nil {
+	functionDNS := ""
+
+	// Only return an error if we're not deleting so that we can delete
+	// the binding even if the route has already been deleted.
+	if err != nil && deletionTimestamp == nil {
 		if errors.IsNotFound(err) {
 			runtime.HandleError(fmt.Errorf("Route %q in namespace %q does not exist", routeName, namespace))
 		}
 		return err
 	}
 
-	// Grab the route
-	functionDNS := route.Status.Domain
+	if route != nil {
+		functionDNS = route.Status.Domain
+		glog.Infof("Found route DNS as %q", functionDNS)
+	}
 	if len(functionDNS) == 0 {
-		runtime.HandleError(fmt.Errorf("Route %q does not have domain", routeName))
+		// Only return an error if we're not deleting so that we can delete
+		// the binding even if the route has already been deleted.
+		if deletionTimestamp == nil {
+			return fmt.Errorf("Route %q does not have domain", routeName)
+		}
+		log.Printf("Route %q does not have domain", routeName)
 	}
 
-	glog.Infof("Found route DNS as '%q'", functionDNS)
-
 	es, err := c.eventSourcesLister.EventSources(namespace).Get(bind.Spec.Trigger.Service)
 	if err != nil {
 		if errors.IsNotFound(err) {
@@ -333,15 +349,6 @@ func (c *Controller) syncHandler(key string) error {
 		return err
 	}
 
-	// See if the binding has been deleted
-	accessor, err := meta.Accessor(bind)
-	if err != nil {
-		log.Printf("Failed to get metadata: %s", err)
-		panic("Failed to get metadata")
-	}
-
-	deletionTimestamp := accessor.GetDeletionTimestamp()
-
 	// If there are conditions or a context do nothing.
 	if (bind.Status.Conditions != nil || bind.Status.BindContext != nil) && deletionTimestamp == nil {
 		glog.Infof("Already has status, skipping")
diff --git a/pkg/sources/gcp_pubsub.go b/pkg/sources/gcp_pubsub.go
index 3e89ba18af7..8caeeb83bb4 100644
--- a/pkg/sources/gcp_pubsub.go
+++ b/pkg/sources/gcp_pubsub.go
@@ -21,6 +21,8 @@ import (
 
 	"github.com/golang/glog"
 	"github.com/google/uuid"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -75,8 +77,13 @@ func (t *GCPPubSubEventSource) Unbind(trigger EventTrigger, bindContext BindCont
 	sub := client.Subscription(subscriptionName)
 	err = sub.Delete(ctx)
 	if err != nil {
-		glog.Warningf("Failed to delete subscription %q : %s", subscriptionName, err)
-		return err
+		glog.Warningf("Failed to delete subscription %q : %s : %+v", subscriptionName, err, err)
+		// Return error only if it's something else than NotFound
+		rpcStatus := status.Convert(err)
+		if rpcStatus.Code() != codes.NotFound {
+			return err
+		}
+		glog.Infof("Subscription %q already deleted", subscriptionName)
 	}
 	return nil
 }
@@ -118,6 +125,13 @@ func (t *GCPPubSubEventSource) Bind(trigger EventTrigger, route string) (*BindCo
 	err = t.createWatcher("bind-system", deploymentName, t.image, projectID, subscriptionName, route)
 	if err != nil {
 		glog.Infof("Failed to create deployment: %v", err)
+
+		// delete the subscription so it's not left floating around.
+		errDelete := sub.Delete(ctx)
+		if errDelete != nil {
+			glog.Infof("Failed to delete subscription while trying to clean up %q : %s", subscriptionName, errDelete)
+		}
+
 		return nil, err
 	}