Fix: Remove volume and volumemount from app resource when unbinding (#950)

When projecting bindings as files, we inject volume and volume mount carrying the same name
as binding resource, but when binding is removed, we wrongly look to remove volume and volume mount
having the name equal to the binding secret.

This change fixes it and introduces the missing acceptance tests.

Signed-off-by: Predrag Knezevic <[email protected]>

Author: pedjak

pkg/reconcile/pipeline/handler/project/impl.go

@@ -237,6 +237,7 @@ func Unbind(ctx pipeline.Context) {
 		return
 	}
 	secretName := ctx.BindingSecretName()
+	bindingName := ctx.BindingName()
 	if secretName == "" {
 		ctx.StopProcessing()
 		return
@@ -254,7 +255,7 @@ func Unbind(ctx pipeline.Context) {
 		volumeResources, found, _ := resources(&corev1.Volume{}, podSpecMap, "volumes")
 		if found {
 			for i, vol := range volumeResources {
-				if val, found, err := unstructured.NestedString(vol, "name"); found && err == nil && val == secretName {
+				if val, found, err := unstructured.NestedString(vol, "name"); found && err == nil && val == bindingName {
 					s := append(volumeResources[:i], volumeResources[i+1:]...)
 					if len(s) == 0 {
 						delete(podSpecMap, "volumes")
@@ -288,7 +289,7 @@ func Unbind(ctx pipeline.Context) {
 			volumeMounts, found, _ := resources(&corev1.VolumeMount{}, container, "volumeMounts")
 			if found {
 				for i, vm := range volumeMounts {
-					if val, found, err := unstructured.NestedString(vm, "name"); found && err == nil && val == secretName {
+					if val, found, err := unstructured.NestedString(vm, "name"); found && err == nil && val == bindingName {
 						s := append(volumeMounts[:i], volumeMounts[i+1:]...)
 						if len(s) == 0 {
 							delete(container, "volumeMounts")

pkg/reconcile/pipeline/handler/project/impl_test.go

@@ -519,12 +519,15 @@ var _ = Describe("Unbind handler", func() {
 			deploymentsUnstructured    []*unstructured.Unstructured
 			deploymentsUnstructuredOld []*unstructured.Unstructured
 			secretName                 string
+			bindingName                string
 		)
 
 		BeforeEach(func() {
 			var apps []pipeline.Application
 			secretName = "secret1"
+			bindingName = "binding1"
 			ctx.EXPECT().BindingSecretName().Return(secretName)
+			ctx.EXPECT().BindingName().Return(bindingName)
 			d1 := deployment("d1", []corev1.Container{
 				{
 					Image: "foo",
@@ -570,7 +573,7 @@ var _ = Describe("Unbind handler", func() {
 					Image: "foo",
 					VolumeMounts: []corev1.VolumeMount{
 						{
-							Name:      secretName,
+							Name:      bindingName,
 							MountPath: "/bla",
 						},
 					},
@@ -581,7 +584,7 @@ var _ = Describe("Unbind handler", func() {
 					Image: "foo",
 					VolumeMounts: []corev1.VolumeMount{
 						{
-							Name:      secretName,
+							Name:      bindingName,
 							MountPath: "/bla",
 						},
 						{
@@ -598,7 +601,7 @@ var _ = Describe("Unbind handler", func() {
 			})
 			d6.Spec.Template.Spec.Volumes = []corev1.Volume{
 				{
-					Name: secretName,
+					Name: bindingName,
 					VolumeSource: corev1.VolumeSource{
 						Secret: &corev1.SecretVolumeSource{
 							SecretName: secretName,
@@ -613,7 +616,7 @@ var _ = Describe("Unbind handler", func() {
 			})
 			d7.Spec.Template.Spec.Volumes = []corev1.Volume{
 				{
-					Name: secretName,
+					Name: bindingName,
 					VolumeSource: corev1.VolumeSource{
 						Secret: &corev1.SecretVolumeSource{
 							SecretName: secretName,

test/acceptance/features/steps/generic_testapp.py

@@ -34,6 +34,10 @@ def get_file_value(self, file_path):
         print(f'file endpoint response: {resp.text} code: {resp.status_code}')
         return resp.text
 
+    def assert_file_not_exist(self, file_path):
+        polling2.poll(lambda: requests.get(url=f"http://{self.route_url}{file_path}"),
+                      check_success=lambda r: r.status_code == 404, step=5, timeout=400, ignore_exceptions=(requests.exceptions.ConnectionError,))
+
 
 @step(u'Generic test application "{application_name}" is running')
 @step(u'Generic test application "{application_name}" is running with binding root as "{bindingRoot}"')
@@ -65,3 +69,8 @@ def check_env_var_existence(context, name):
 def check_file_value(context, file_path):
     value = context.text.strip()
     polling2.poll(lambda: context.application.get_file_value(file_path) == value, step=5, timeout=400)
+
+
+@step(u'File "{file_path}" is unavailable in application pod')
+def check_file_unavailable(context, file_path):
+    context.application.assert_file_not_exist(file_path)

test/acceptance/features/steps/steps.py

@@ -532,10 +532,14 @@ def validate_secret_empty(context):
         assert False, "sbr_name not in context"
 
 
+def assert_generation(context, count):
+    context.latest_application_generation = context.application.get_generation()
+    return context.latest_application_generation - context.original_application_generation == int(count)
+
+
 @then(u'The application got redeployed {count} times so far')
 def check_generation(context, count):
-    context.latest_application_generation = context.application.get_generation()
-    assert context.latest_application_generation - context.original_application_generation == int(count), "Unexpected number of application redeployments"
+    polling2.poll(lambda: assert_generation(context, count), step=5, timeout=400)
 
 
 @then(u'The application does not get redeployed again with {time} minutes')

test/acceptance/features/unbindAppToService.feature

@@ -107,3 +107,52 @@ Feature: Unbind an application from a service
 
         Then The env var "BACKEND_HOST" is not available to the application
         And The env var "BACKEND_USERNAME" is not available to the application
+
+    Scenario: Remove bindings projected as files from generic test application
+        Given Generic test application "remove-bindings-as-files-app" is running
+        * The Custom Resource is present
+            """
+            apiVersion: "stable.example.com/v1"
+            kind: Backend
+            metadata:
+                name: remove-bindings-as-files-app-backend
+                annotations:
+                    "service.binding/host": "path={.spec.host}"
+                    "service.binding/port": "path={.spec.port}"
+            spec:
+                host: example.common
+                port: 8080
+            """
+        * Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: remove-bindings-as-files-app-sb
+            spec:
+                services:
+                -   group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: remove-bindings-as-files-app-backend
+
+                application:
+                    name: remove-bindings-as-files-app
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        * Service Binding "remove-bindings-as-files-app-sb" is ready
+
+        * Content of file "/bindings/remove-bindings-as-files-app-sb/host" in application pod is
+            """
+            example.common
+            """
+        * Content of file "/bindings/remove-bindings-as-files-app-sb/port" in application pod is
+            """
+            8080
+            """
+        When Service Binding "remove-bindings-as-files-app-sb" is deleted
+        Then The application got redeployed 2 times so far
+        * File "/bindings/remove-bindings-as-files-app-sb/host" is unavailable in application pod
+        * File "/bindings/remove-bindings-as-files-app-sb/port" is unavailable in application pod