Regression fix: refer multiple services in custom mapping templates (#…

…955)

service structs are initialized inside a `for range` loop, and the loop variable is passed as a reference to each of service structs.
However, with each new iteration its value changes, but the reference remains the same, leading that the value of the last iteration
is passed to all service structs eventually.

* do not pass loop variable to service structs
* unit tests were extended to detect regressions in the future
* `Bind two backend services by creating 1 SBR to a single application` acceptance test was modified to detect regressions in the future

Signed-off-by: Predrag Knezevic <pknezevi@redhat.com>

pkg/reconcile/pipeline/context/impl.go

@@ -91,7 +91,9 @@ func (i *impl) Mappings() map[string]string {
 
 func (i *impl) Services() ([]pipeline.Service, error) {
 	if i.services == nil {
-		for _, serviceRef := range i.serviceBinding.Spec.Services {
+		serviceRefs := i.serviceBinding.Spec.Services
+		for idx := 0; idx<len(serviceRefs); idx++ {
+			serviceRef := serviceRefs[idx]
 			gvr, err := i.typeLookup.ResourceForReferable(&serviceRef)
 			if err != nil {
 				return nil, err
@@ -107,8 +109,8 @@ func (i *impl) Services() ([]pipeline.Service, error) {
 		}
 	}
 	services := make([]pipeline.Service, len(i.services))
-	for l, s := range i.services {
-		services[l] = s
+	for idx := 0; idx<len(i.services); idx++ {
+		services[idx] = i.services[idx]
 	}
 	return services, nil
 }

pkg/reconcile/pipeline/context/impl_test.go

@@ -128,11 +128,13 @@ var _ = Describe("Context", func() {
 		var (
 			defServiceBinding = func(name string, namespace string, refs ...v1alpha1.Ref) *v1alpha1.ServiceBinding {
 				var services []v1alpha1.Service
-				for _, ref := range refs {
+				for idx, ref := range refs {
+					id := fmt.Sprintf("id%v", idx)
 					services = append(services, v1alpha1.Service{
 						NamespacedRef: v1alpha1.NamespacedRef{
 							Ref: ref,
 						},
+						Id: &id,
 					})
 				}
 				sb := &v1alpha1.ServiceBinding{
@@ -181,6 +183,8 @@ var _ = Describe("Context", func() {
 					}
 					Expect(serviceImpl.client).To(Equal(client))
 					Expect(serviceImpl.groupVersionResource).To(Equal(gvr))
+					Expect(serviceImpl.serviceRef.Name).To(Equal(tc.serviceRefs[i].Name))
+					Expect(*serviceImpl.serviceRef.Id).To(Equal(fmt.Sprintf("id%v", i)))
 				}
 			},
 			Entry("single service", &testCase{

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

@@ -22,8 +22,8 @@ var _ = Describe("Mapping Handler", func() {
 	BeforeEach(func() {
 		mockCtrl = gomock.NewController(GinkgoT())
 		ctx = mocks.NewMockContext(mockCtrl)
-		services = []*mocks.MockService{mocks.NewMockService(mockCtrl), mocks.NewMockService(mockCtrl)}
-		ctx.EXPECT().Services().Return([]pipeline.Service{services[0], services[1]}, nil)
+		services = []*mocks.MockService{mocks.NewMockService(mockCtrl), mocks.NewMockService(mockCtrl), mocks.NewMockService(mockCtrl)}
+		ctx.EXPECT().Services().Return([]pipeline.Service{services[0], services[1], services[2]}, nil)
 		bindingItems = []*pipeline.BindingItem{
 			{
 				Name:  "foo",
@@ -42,6 +42,16 @@ var _ = Describe("Mapping Handler", func() {
 		services[0].EXPECT().Id().Return(&srvId).MinTimes(1)
 		services[0].EXPECT().Resource().Return(u)
 
+		u2 := &unstructured.Unstructured{Object: map[string]interface{}{
+			"bar2": "bla2",
+		}}
+		u2.SetName("n1")
+		u2.SetNamespace("ns1")
+
+		srvId2 := "id2"
+		services[2].EXPECT().Id().Return(&srvId2).MinTimes(1)
+		services[2].EXPECT().Resource().Return(u2)
+
 		services[1].EXPECT().Id().Return(nil).MinTimes(1)
 	})
 
@@ -56,6 +66,7 @@ var _ = Describe("Mapping Handler", func() {
 		mapping.Handle(ctx)
 	},
 		Entry("property referred via service id", "{{ .id1.bar }}", "bla"),
+		Entry("property referred via service id", "{{ .id1.bar }}_{{ .id2.metadata.name }}", "bla_n1"),
 		Entry("use existing bindins", "{{ .foo }}_{{ .foo2 }}", "val1_val2"))
 
 	DescribeTable("failed processing", func(template string) {
@@ -70,4 +81,5 @@ var _ = Describe("Mapping Handler", func() {
 		mapping.Handle(ctx)
 	},
 		Entry("bad template", "{{ .id1.bar "))
+
 })

test/acceptance/features/bindAppToMultipleServices.feature

@@ -118,16 +118,25 @@ Feature: Bind a single application to multiple services
                     group: apps
                     version: v1
                     resource: deployments
+                mappings:
+                - name: FOO
+                  value: '{{ .db1.metadata.name }}_{{ .db2.metadata.name }}'
+                - name: FOO2
+                  value: '{{ .db1.metadata.name }}_{{ .db2.kind }}'
                 services:
                 -   group: stable.example.com
                     version: v1
                     kind: Backend
                     name: internal-db-1sbr
+                    id: db1
                 -   group: stable.example.com
                     version: v1
                     kind: Backend
                     name: external-db-1sbr
+                    id: db2
             """
         Then Service Binding "binding-request-1sbr" is ready
         And The application env var "BACKEND_HOST_INTERNAL_DB" has value "internal.db.stable.example.com"
         And The application env var "BACKEND_HOST_EXTERNAL_DB" has value "external.db.stable.example.com"
+        And The application env var "FOO" has value "internal-db-1sbr_external-db-1sbr"
+        And The application env var "FOO2" has value "internal-db-1sbr_Backend"