return gone on unbind from non-existent templateinstance

Author: bparees

pkg/templateservicebroker/servicebroker/unbind.go

@@ -43,19 +43,24 @@ func (b *Broker) Unbind(u user.Info, instanceID, bindingID string) *api.Response
 		return api.Forbidden(err)
 	}
 
+	status := http.StatusGone
 	templateInstance, err := b.templateclient.TemplateInstances(namespace).Get(brokerTemplateInstance.Spec.TemplateInstance.Name, metav1.GetOptions{})
 	if err != nil {
-		return api.InternalServerError(err)
+		// Tolerate NotFound errors in case the user deleted the templateinstance manually.  We do not
+		// want to leave the system in a state where unbind cannot proceed because then the user cannot
+		// deprovision either(you must unbind before deprovisioning).  So just proceed as if we found it.
+		if !kerrors.IsNotFound(err) {
+			return api.InternalServerError(err)
+		}
 	}
-	if strings.ToLower(templateInstance.Spec.Template.Annotations[templateapi.BindableAnnotation]) == "false" {
+	if templateInstance != nil && strings.ToLower(templateInstance.Spec.Template.Annotations[templateapi.BindableAnnotation]) == "false" {
 		return api.BadRequest(errors.New("provisioned service is not bindable"))
 	}
 
 	// The OSB API requires this function to be idempotent (restartable).  If
 	// any actual change was made, per the spec, StatusOK is returned, else
 	// StatusGone.
 
-	status := http.StatusGone
 	for i := 0; i < len(brokerTemplateInstance.Spec.BindingIDs); i++ {
 		for i < len(brokerTemplateInstance.Spec.BindingIDs) && brokerTemplateInstance.Spec.BindingIDs[i] == bindingID {
 			brokerTemplateInstance.Spec.BindingIDs = append(brokerTemplateInstance.Spec.BindingIDs[:i], brokerTemplateInstance.Spec.BindingIDs[i+1:]...)
@@ -65,6 +70,8 @@ func (b *Broker) Unbind(u user.Info, instanceID, bindingID string) *api.Response
 	if status == http.StatusOK { // binding found; remove it
 		// end users are not expected to have access to BrokerTemplateInstance
 		// objects; SAR on the TemplateInstance instead.
+		// Note that this specific templateinstance object might not actually exist anymore, but the SAR check
+		// is still valid to confirm the user can update templateinstances in this namespace.
 		if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorizationv1.ResourceAttributes{
 			Namespace: namespace,
 			Verb:      "update",

test/extended/templates/templateservicebroker_e2e.go

@@ -374,7 +374,21 @@ var _ = g.Describe("[Conformance][templates] templateservicebroker end-to-end te
 			provision()
 			bind()
 			unbind()
+			// unbinding a second time should result in a gone message, but not an error
+			unbind()
 			deprovision()
+
+			/*
+			     Reenable once the TSB changes have made it into a published image
+			   			provision()
+			   			bind()
+			   			g.By("deleting the template instance that was bound")
+			   			err := cli.Run("delete").Args("templateinstance", "--all").Execute()
+			   			o.Expect(err).NotTo(o.HaveOccurred())
+			   			unbind()
+			   			// unbinding a second time should result in a gone message, but not an error
+			   			unbind()
+			*/
 		})
 	})
 })