Remove 'mountPath' support (#1035)

Reportedly, mountPath hasn't seen a lot of use, and it doesn't make
sense to support it for GA.  Therefore, unless we want to commit to it
for GA, we should remove it now.

Signed-off-by: Andy Sadler <[email protected]>

Author: sadlerap

apis/binding/v1alpha1/servicebinding_types.go

@@ -30,18 +30,6 @@ var templates = map[string]string{
 
 // ServiceBindingSpec defines the desired state of ServiceBinding.
 type ServiceBindingSpec struct {
-	// MountPath specifies the path inside the app container where bindings
-	// will be mounted.  The environment variable `SERVICE_BINDING_ROOT`
-	// has higher precedence than this option, and setting it within an
-	// application to be bound will cause MountPath to be ignored and will
-	// always mount resources as files.  If it isn't set, then it is
-	// automatically set to the value of MountPath.  If neither MountPath
-	// nor `SERVICE_BINDING_ROOT` are set, this field defaults to
-	// `/bindings`, and `SERVICE_BINDING_ROOT` is also set.  This results
-	// in the file being mounted under the directory specified.
-	// +optional
-	MountPath string `json:"mountPath,omitempty"`
-
 	// NamingStrategy defines custom string template for preparing binding
 	// names.  It can be set to pre-defined strategies: `none`,
 	// `lowercase`, or `uppercase`.  Otherwise, it is treated as a custom
@@ -70,8 +58,9 @@ type ServiceBindingSpec struct {
 	DetectBindingResources bool `json:"detectBindingResources,omitempty"`
 
 	// BindAsFiles makes the binding values available as files in the
-	// application's container.  See the MountPath attribute description
-	// for more details.
+	// application's container.  By default, values are mounted under the path
+	// "/bindings"; this can be changed by setting the SERVICE_BINDING_ROOT
+	// environment variable.
 	// +optional
 	// +kubebuilder:default:=true
 	BindAsFiles bool `json:"bindAsFiles"`

config/crd/bases/binding.operators.coreos.com_servicebindings.yaml

@@ -133,8 +133,9 @@ spec:
               bindAsFiles:
                 default: true
                 description: BindAsFiles makes the binding values available as files
-                  in the application's container.  See the MountPath attribute description
-                  for more details.
+                  in the application's container.  By default, values are mounted
+                  under the path "/bindings"; this can be changed by setting the SERVICE_BINDING_ROOT
+                  environment variable.
                 type: boolean
               detectBindingResources:
                 description: DetectBindingResources is a flag that, when set to true,
@@ -160,17 +161,6 @@ spec:
                   - value
                   type: object
                 type: array
-              mountPath:
-                description: MountPath specifies the path inside the app container
-                  where bindings will be mounted.  The environment variable `SERVICE_BINDING_ROOT`
-                  has higher precedence than this option, and setting it within an
-                  application to be bound will cause MountPath to be ignored and will
-                  always mount resources as files.  If it isn't set, then it is automatically
-                  set to the value of MountPath.  If neither MountPath nor `SERVICE_BINDING_ROOT`
-                  are set, this field defaults to `/bindings`, and `SERVICE_BINDING_ROOT`
-                  is also set.  This results in the file being mounted under the directory
-                  specified.
-                type: string
               namingStrategy:
                 description: 'NamingStrategy defines custom string template for preparing
                   binding names.  It can be set to pre-defined strategies: `none`,

docs/User_Guide.md

@@ -283,51 +283,9 @@ bindings
 │   ├── COCKROACHDB_CONF_PORT
 ```
 
-Instead of `/bindings`, you can specify a custom binding root path by specifying the same in `spec.mountPath`, example,
+Setting `spec.bindAsFiles` to `false` (default: `true`) enables injecting gathered bindings as environment variables into the application/workload instead of injecting this data as files.
 
-``` yaml
-apiVersion: binding.operators.coreos.com/v1alpha1
-kind: ServiceBinding
-metadata:
-  name: binding-request
-  namespace: service-binding-demo
-spec:
-  bindAsFiles: true
-  application:
-    name: java-app
-    group: apps
-    version: v1
-    resource: deployments
-  services:
-  - group: charts.helm.k8s.io
-    version: v1alpha1
-    kind: Cockroachdb
-    name: db-demo
-    id: db_1
-  mounthPath: '/bindings/accounts-db' # User configurable binding root
-```
-
-Here's how the mount paths would look like, where applicable:
-
-```
-bindings
-├── accounts-db
-│   ├── COCKROACHDB_CLUSTERIP
-│   ├── COCKROACHDB_CONF_PORT
-```
-
-Setting `spec.bindAsFiles` to `false` (default: `true`) enables injecting gathered bindings as env variables into the application/workload.
-
-For determining the folder where bindings should be injected, we can specify the destination using `spec.mountPath` or we can use `SERVICE_BINDING_ROOT` environment variable. If both are set then the `SERVICE_BINDING_ROOT` environment variable takes the higher precedence.
-
-The following table summarizes how the final bind path is computed:
-
-| spec.mountPath  | SERVICE_BINDING_ROOT | Final Bind Path                      |
-| --------------- | ---------------------| -------------------------------------|
-| nil             | non-existent         | /bindings/ServiceBinding_Name        |
-| nil             | /some/path/root      | /some/path/root/ServiceBinding_Name  |
-| /home/foo       | non-existent         | /home/foo                            |
-| /home/foo       | /some/path/root      | /some/path/root/ServiceBinding_Name  |
+For determining the folder where bindings should be injected, we can use the `SERVICE_BINDING_ROOT` environment variable.  This provides a method for overriding the path that injected configuration data will be exposed.
 
 ## Custom naming strategy
 Binding names declared through annotations or CSV descriptors are processed before injected into the application according to the following strategy

pkg/reconcile/pipeline/api.go

@@ -110,9 +110,6 @@ type Context interface {
 	// Return true if bindings should be projected as files inside application containers
 	BindAsFiles() bool
 
-	// Path where bindings should be mounted inside application containers
-	MountPath() string
-
 	// Template that should be applied on collected binding names, prior projection
 	NamingTemplate() string
 

pkg/reconcile/pipeline/context/impl.go

@@ -135,10 +135,6 @@ func (i *bindingImpl) EnvBindings() []*pipeline.EnvBinding {
 	return make([]*pipeline.EnvBinding, 0)
 }
 
-func (i *bindingImpl) MountPath() string {
-	return i.serviceBinding.Spec.MountPath
-}
-
 func (i *bindingImpl) Mappings() map[string]string {
 	result := make(map[string]string)
 	for _, m := range i.serviceBinding.Spec.Mappings {

pkg/reconcile/pipeline/context/spec_binding_impl.go

@@ -185,10 +185,6 @@ func (s *specImpl) BindAsFiles() bool {
 	return true
 }
 
-func (s *specImpl) MountPath() string {
-	return ""
-}
-
 func (s *specImpl) NamingTemplate() string {
 	return ""
 }

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

@@ -374,13 +374,8 @@ func mountPath(container map[string]interface{}, ctx pipeline.Context) (string,
 		}
 	}
 
-	mp := ctx.MountPath()
-	if mp == "" {
-		bindingRoot = "/bindings"
-		mp = path.Join(bindingRoot, ctx.BindingName())
-	} else {
-		return mp, nil
-	}
+	bindingRoot = "/bindings"
+	mp := path.Join(bindingRoot, ctx.BindingName())
 
 	u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&corev1.EnvVar{
 		Name:  bindingRootEnvVar,

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

@@ -474,26 +474,31 @@ var _ = Describe("Inject bindings as files", func() {
 			}
 
 			ctx.EXPECT().Applications().Return(apps, nil)
-			ctx.EXPECT().MountPath().Return("/bla").AnyTimes()
 			ctx.EXPECT().BindingName().Return(bindingName).AnyTimes()
 		})
 		It("should mount binding secret as volume", func() {
 			project.BindingsAsFiles(ctx)
 			for i, old := range deploymentsUnstructuredOld {
 				Expect(deploymentsUnstructured[i]).NotTo(Equal(old))
 			}
-			exoectedD1 := deployment("d1", []corev1.Container{
+			expectedD1 := deployment("d1", []corev1.Container{
 				{
 					Image: "foo",
+					Env: []corev1.EnvVar{
+						{
+							Name:  "SERVICE_BINDING_ROOT",
+							Value: "/bindings",
+						},
+					},
 					VolumeMounts: []corev1.VolumeMount{
 						{
 							Name:      bindingName,
-							MountPath: "/bla",
+							MountPath: "/bindings/sb1",
 						},
 					},
 				},
 			})
-			exoectedD1.Spec.Template.Spec.Volumes = []corev1.Volume{
+			expectedD1.Spec.Template.Spec.Volumes = []corev1.Volume{
 				{
 					Name: bindingName,
 					VolumeSource: corev1.VolumeSource{
@@ -565,10 +570,16 @@ var _ = Describe("Inject bindings as files", func() {
 			expectedD4 := deployment("d4", []corev1.Container{
 				{
 					Image: "foo",
+					Env: []corev1.EnvVar{
+						{
+							Name:  "SERVICE_BINDING_ROOT",
+							Value: "/bindings",
+						},
+					},
 					VolumeMounts: []corev1.VolumeMount{
 						{
 							Name:      bindingName,
-							MountPath: "/bla",
+							MountPath: "/bindings/sb1",
 						},
 					},
 				},
@@ -599,11 +610,15 @@ var _ = Describe("Inject bindings as files", func() {
 							Name:  "SOME_ENV",
 							Value: "val1",
 						},
+						{
+							Name:  "SERVICE_BINDING_ROOT",
+							Value: "/bindings",
+						},
 					},
 					VolumeMounts: []corev1.VolumeMount{
 						{
 							Name:      bindingName,
-							MountPath: "/bla",
+							MountPath: "/bindings/sb1",
 						},
 					},
 				},
@@ -621,10 +636,16 @@ var _ = Describe("Inject bindings as files", func() {
 			expectedD6 := deployment("d6", []corev1.Container{
 				{
 					Image: "foo",
+					Env: []corev1.EnvVar{
+						{
+							Name:  "SERVICE_BINDING_ROOT",
+							Value: "/bindings",
+						},
+					},
 					VolumeMounts: []corev1.VolumeMount{
 						{
 							Name:      bindingName,
-							MountPath: "/bla",
+							MountPath: "/bindings/sb1",
 						},
 					},
 				},
@@ -639,7 +660,7 @@ var _ = Describe("Inject bindings as files", func() {
 					},
 				},
 			}
-			for i, expectedDeployment := range []*appsv1.Deployment{exoectedD1, expectedD2, expectedD3, expectedD4, expectedD5, expectedD6, expectedD6, expectedD6, expectedD6} {
+			for i, expectedDeployment := range []*appsv1.Deployment{expectedD1, expectedD2, expectedD3, expectedD4, expectedD5, expectedD6, expectedD6, expectedD6, expectedD6} {
 				d := &appsv1.Deployment{}
 				err := runtime.DefaultUnstructuredConverter.FromUnstructured(deploymentsUnstructured[i].Object, d)
 				Expect(err).NotTo(HaveOccurred())

pkg/reconcile/pipeline/mocks/mocks_pipeline.go

@@ -175,7 +175,6 @@ Feature: Bind an application to a service using custom naming strategies
                 name: binding-request-custom-file-naming
             spec:
                 namingStrategy: "PREFIX_{{ .service.kind | upper }}_{{ .name | upper }}_SUFFIX"
-                mountPath: "/foo/bar"
                 bindAsFiles: true
                 application:
                     name: myapp-custom-file-naming
@@ -189,7 +188,7 @@ Feature: Bind an application to a service using custom naming strategies
                     name: backend-custom-file-naming
             """
         Then Service Binding "binding-request-custom-file-naming" is ready
-        And Content of file "/foo/bar/PREFIX_BACKEND_HOST_CROSS_NS_SERVICE_SUFFIX" in application pod is
+        And Content of file "/bindings/binding-request-custom-file-naming/PREFIX_BACKEND_HOST_CROSS_NS_SERVICE_SUFFIX" in application pod is
             """
             cross.ns.service.stable.example.com
             """