Repository: redhat-developer/service-binding-operator
Pull request #1023 (2021-09-16 20:31:10)

Treat invalid "elementType" and "objectType" annotations as errors, not panics
### Motivation

If a service has an invalid service binding annotation set, we shouldn't attempt to bind the service.  However, current behavior catches most cases (such as an invalid path), but errors currently are ignored, and when either `elementType` or `objectType` isn't valid, we panic.  This could cause issues for users.

### Changes

Rather than panicking, we should fail the binding request and report any errors that are thrown.

### Testing

Attempting to bind against a service with an garbage annotation should now fail and report errors in the binding status.  For example, ```service.binding/credentials: path={.status.dbCredentials},elementType=asdf``` should fail.  This currently implements unit and acceptance tests for both `elementType` and `objectType` errors.  It also adds an acceptance test for invalid paths for the sake of regression testing.

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **sadlerap** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["sadlerap"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) Report
> Merging [#1023](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) (bcf36f5) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/00c602da6d96cc4d15608e513580d15ca3ed3951?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) (00c602d) will **decrease** coverage by `0.16%`.
> The diff coverage is `45.45%`.

[![Impacted file tree graph](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023/graphs/tree.svg?width=650&height=150&src=pr&token=CICBeVWlrC&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)

```diff
@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
- Coverage   57.02%   56.85%   -0.17%     
==========================================
  Files          28       28              
  Lines        1552     1560       +8     
==========================================
+ Hits          885      887       +2     
- Misses        552      557       +5     
- Partials      115      116       +1     
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) | Coverage Δ | |
|---|---|---|
| [pkg/reconcile/pipeline/handler/collect/impl.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer#diff-cGtnL3JlY29uY2lsZS9waXBlbGluZS9oYW5kbGVyL2NvbGxlY3QvaW1wbC5nbw==) | `79.16% <40.00%> (-3.34%)` | :arrow_down: |
| [pkg/binding/annotationmapper.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer#diff-cGtnL2JpbmRpbmcvYW5ub3RhdGlvbm1hcHBlci5nbw==) | `85.91% <100.00%> (+1.40%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer). Last update [00c602d...bcf36f5](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/1023?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer).

openshift-ci[bot]: @sadlerap: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.8-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.8-acceptance/1438909907897159680) | `/test 4.8-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=1023). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:sadlerap).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @sadlerap: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.8-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.8-acceptance/1438909907897159680) | `/test 4.8-acceptance`
ci/prow/4.7-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1438909907809079296) | `/test 4.7-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=1023). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:sadlerap).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @sadlerap: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.8-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.8-acceptance/1438909907897159680) | `/test 4.8-acceptance`
ci/prow/4.7-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1438909907809079296) | `/test 4.7-acceptance`
ci/prow/4.9-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.9-acceptance/1438909907859410944) | `/test 4.9-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=1023). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:sadlerap).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @sadlerap: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.8-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.8-acceptance/1438909907897159680) | `/test 4.8-acceptance`
ci/prow/4.7-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1438909907809079296) | `/test 4.7-acceptance`
ci/prow/4.9-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.9-acceptance/1438909907859410944) | `/test 4.9-acceptance`
ci/prow/4.6-acceptance | 957d3ceaa4bb05166faaae62fab1c848bc58f9c4 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.6-acceptance/1438909907532255232) | `/test 4.6-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=1023). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:sadlerap).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @sadlerap: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
ci/prow/4.7-acceptance | 86e2d08d2c8b8ba48620a817656cd33fc05c338c | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/1023/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1440783561211777024) | true | `/test 4.7-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=1023). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:sadlerap).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
sadlerap: /retest
pedjak: /lgtm
/approve
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-developer/service-binding-operator/pull/1023#issuecomment-927270433" title="Approved">pedjak</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)~~ [pedjak]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/binding/annotationmapper.go b/pkg/binding/annotationmapper.go
index 4fed6085..e68ab2f5 100644
--- a/pkg/binding/annotationmapper.go
+++ b/pkg/binding/annotationmapper.go
@@ -2,9 +2,10 @@ package binding
 
 import (
 	"fmt"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
 	"github.com/pkg/errors"
 )
 
@@ -26,12 +27,13 @@ var _ DefinitionBuilder = (*annotationBackedDefinitionBuilder)(nil)
 type modelKey string
 
 const (
-	pathModelKey        modelKey = "path"
-	objectTypeModelKey  modelKey = "objectType"
-	sourceKeyModelKey   modelKey = "sourceKey"
-	sourceValueModelKey modelKey = "sourceValue"
-	elementTypeModelKey modelKey = "elementType"
-	AnnotationPrefix             = "service.binding"
+	pathModelKey                    modelKey = "path"
+	objectTypeModelKey              modelKey = "objectType"
+	sourceKeyModelKey               modelKey = "sourceKey"
+	sourceValueModelKey             modelKey = "sourceValue"
+	elementTypeModelKey             modelKey = "elementType"
+	AnnotationPrefix                         = "service.binding"
+	ProvisionedServiceAnnotationKey          = "servicebinding.io/provisioned-service"
 )
 
 func NewDefinitionBuilder(annotationName string, annotationValue string, configMapReader UnstructuredResourceReader, secretReader UnstructuredResourceReader) *annotationBackedDefinitionBuilder {
@@ -45,26 +47,44 @@ func NewDefinitionBuilder(annotationName string, annotationValue string, configM
 	}
 }
 
-func (m *annotationBackedDefinitionBuilder) outputName() (string, error) {
-	// bail out in the case the annotation name doesn't start with "service.binding"
-	if m.name != AnnotationPrefix && !strings.HasPrefix(m.name, AnnotationPrefix+"/") {
-		return "", fmt.Errorf("can't process annotation with name %q", m.name)
+func (m *annotationBackedDefinitionBuilder) isServiceBindingAnnotation() (bool, error) {
+	if m.name == ProvisionedServiceAnnotationKey {
+		return false, nil
+	}
+
+	if m.name == AnnotationPrefix {
+		return true, nil
+	} else if strings.HasPrefix(m.name, AnnotationPrefix) {
+		if strings.HasPrefix(m.name, AnnotationPrefix+"/") {
+			return true, nil
+		}
+
+		// it starts with AnnotationPrefix, but has extra text at the end not
+		// separated by a /, so treat it as an error
+		return false, fmt.Errorf("can't process annotation with name %q", m.name)
 	}
 
-	if p := strings.SplitN(m.name, "/", 2); len(p) > 1 && len(p[1]) > 0 {
-		return p[1], nil
+	// bail out when the annotation name doesn't start with "service.binding"
+	return false, nil
+}
+
+func (m *annotationBackedDefinitionBuilder) outputName() string {
+
+	if p := strings.SplitN(m.name, "/", 2); len(p) == 2 && len(p[1]) > 0 {
+		return p[1]
 	}
 
-	return "", nil
+	return ""
 }
 
 func (m *annotationBackedDefinitionBuilder) Build() (Definition, error) {
 
-	outputName, err := m.outputName()
-	if err != nil {
+	if valid, err := m.isServiceBindingAnnotation(); !valid || err != nil {
 		return nil, err
 	}
 
+	outputName := m.outputName()
+
 	mod, err := newModel(m.value)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not create binding model for annotation key %s and value %s", m.name, m.value)
@@ -128,6 +148,5 @@ func (m *annotationBackedDefinitionBuilder) Build() (Definition, error) {
 			sourceValue: mod.sourceValue,
 		}, nil
 	}
-
-	panic(fmt.Sprintf("Annotation %s=%s not implemented!", m.name, m.value))
+	return nil, fmt.Errorf("Annotation %s: %s not implemented!", m.name, m.value)
 }
diff --git a/pkg/reconcile/pipeline/handler/collect/impl.go b/pkg/reconcile/pipeline/handler/collect/impl.go
index b31b2019..6c18fb7f 100644
--- a/pkg/reconcile/pipeline/handler/collect/impl.go
+++ b/pkg/reconcile/pipeline/handler/collect/impl.go
@@ -27,7 +27,8 @@ const (
 	ErrorReadingBindingReason    = "ErrorReadingBinding"
 	ErrorReadingSecret           = "ErrorReadingSecret"
 
-	ValueNotFound = "ValueNotFound"
+	ValueNotFound     = "ValueNotFound"
+	InvalidAnnotation = "InvalidAnnotation"
 )
 
 func PreFlight(ctx pipeline.Context) {
@@ -65,9 +66,14 @@ func BindingDefinitions(ctx pipeline.Context) {
 		for k, v := range anns {
 			definition, err := makeBindingDefinition(k, v, ctx)
 			if err != nil {
-				continue
+				condition := notCollectionReadyCond(InvalidAnnotation, fmt.Errorf("Failed to create binding definition from \"%v: %v\": %v", k, v, err))
+				ctx.SetCondition(condition)
+				ctx.Error(err)
+				ctx.StopProcessing()
+			}
+			if definition != nil {
+				service.AddBindingDef(definition)
 			}
-			service.AddBindingDef(definition)
 		}
 	}
 }
@@ -96,8 +102,6 @@ func BindingItems(ctx pipeline.Context) {
 	}
 }
 
-const ProvisionedServiceAnnotationKey = "servicebinding.io/provisioned-service"
-
 func ProvisionedService(ctx pipeline.Context) {
 	services, _ := ctx.Services()
 
@@ -126,7 +130,7 @@ func ProvisionedService(ctx pipeline.Context) {
 			if crd == nil {
 				continue
 			}
-			v, ok := crd.Resource().GetAnnotations()[ProvisionedServiceAnnotationKey]
+			v, ok := crd.Resource().GetAnnotations()[binding.ProvisionedServiceAnnotationKey]
 			if ok && v == "true" {
 				requestRetry(ctx, ErrorReadingBindingReason, fmt.Errorf("CRD of service %v/%v indicates provisioned service, but no secret name provided under .status.binding.name", res.GetNamespace(), res.GetName()))
 				return
@@ -255,9 +259,7 @@ func collectItems(prefix string, ctx pipeline.Context, service pipeline.Service,
 			if mapVal := v.MapIndex(n).Interface(); mapVal != nil {
 				collectItems(p, ctx, service, n, mapVal)
 			} else {
-				condition := apis.Conditions().NotCollectionReady().
-					Msg(fmt.Sprintf("Value for key %v_%v not found", prefix+k.String(), n.String())).
-					Reason(ValueNotFound).Build()
+				condition := notCollectionReadyCond(ValueNotFound, fmt.Errorf("Value for key %v_%v not found", prefix+k.String(), n.String()))
 				ctx.SetCondition(condition)
 				ctx.Error(ErrorValueNotFound)
 				ctx.StopProcessing()


Test patch:
diff --git a/pkg/binding/annotationmapper_test.go b/pkg/binding/annotationmapper_test.go
index 4daa3889..8d693444 100644
--- a/pkg/binding/annotationmapper_test.go
+++ b/pkg/binding/annotationmapper_test.go
@@ -41,9 +41,17 @@ func TestAnnotationBackedBuilderInvalidAnnotation(t *testing.T) {
 			},
 		},
 		{
-			description: "other prefix supplied",
+			description: "invalid element type",
 			builder: &annotationBackedDefinitionBuilder{
-				name: "other.prefix",
+				name:  "service.binding/databaseField",
+				value: "path={.status.dbCredential},elementType=asdf",
+			},
+		},
+		{
+			description: "invalid object type",
+			builder: &annotationBackedDefinitionBuilder{
+				name:  "service.binding/username",
+				value: "path={.status.dbCredential},objectType=asdf,valueKey=username",
 			},
 		},
 	}
@@ -77,7 +85,22 @@ func TestAnnotationBackedBuilderValidAnnotations(t *testing.T) {
 				},
 			},
 		},
-
+		{
+			description: "Ignore non-service binding annotations",
+			builder: &annotationBackedDefinitionBuilder{
+				name:  "foo",
+				value: "bar",
+			},
+			expectedValue: nil,
+		},
+		{
+			description: "Ignore provisioned service binding annotations",
+			builder: &annotationBackedDefinitionBuilder{
+				name:  ProvisionedServiceAnnotationKey,
+				value: "true",
+			},
+			expectedValue: nil,
+		},
 		{
 			description: "string definition",
 			builder: &annotationBackedDefinitionBuilder{
diff --git a/pkg/reconcile/pipeline/handler/collect/impl_test.go b/pkg/reconcile/pipeline/handler/collect/impl_test.go
index e85520d2..2e7801dc 100644
--- a/pkg/reconcile/pipeline/handler/collect/impl_test.go
+++ b/pkg/reconcile/pipeline/handler/collect/impl_test.go
@@ -156,7 +156,7 @@ var _ = Describe("Collect Binding Definitions", func() {
 
 				serviceContent.SetAnnotations(map[string]string{
 					"foo":                                   "bar",
-					collect.ProvisionedServiceAnnotationKey: "true",
+					binding.ProvisionedServiceAnnotationKey: "true",
 					"service.binding/foo":                   "path={.status.foo},objectType=Secret,sourceValue=username",
 					"service.binding/foo2":                  "path={.status.foo2},objectType=Secret,sourceValue=username",
 				})
@@ -420,6 +420,51 @@ var _ = Describe("Collect Binding Data", func() {
 			)
 		})
 
+		Context("on invalid annotations", func() {
+			var (
+				service        *mocks.MockService
+				serviceContent *unstructured.Unstructured
+			)
+			BeforeEach(func() {
+				service = mocks.NewMockService(mockCtrl)
+				serviceContent = &unstructured.Unstructured{}
+				service.EXPECT().CustomResourceDefinition().Return(nil, nil)
+				service.EXPECT().Resource().Return(serviceContent)
+
+				ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil)
+			})
+
+			It("should error when elementType is invalid", func() {
+				serviceContent.SetAnnotations(map[string]string{
+					"service.binding/foo": "path={.status.foo},elementType=asdf",
+				})
+
+				condition := apis.Conditions().NotCollectionReady().
+					Msg("Failed to create binding definition from \"service.binding/foo: path={.status.foo},elementType=asdf\": Annotation service.binding/foo: path={.status.foo},elementType=asdf not implemented!").
+					Reason(collect.InvalidAnnotation).Build()
+				ctx.EXPECT().SetCondition(condition)
+				ctx.EXPECT().Error(errors.New("Annotation service.binding/foo: path={.status.foo},elementType=asdf not implemented!"))
+				ctx.EXPECT().StopProcessing()
+
+				collect.BindingDefinitions(ctx)
+			})
+
+			It("should error when objectType is invalid", func() {
+				serviceContent.SetAnnotations(map[string]string{
+					"service.binding/foo": "path={.status.foo},objectType=asdf",
+				})
+
+				condition := apis.Conditions().NotCollectionReady().
+					Msg("Failed to create binding definition from \"service.binding/foo: path={.status.foo},objectType=asdf\": Annotation service.binding/foo: path={.status.foo},objectType=asdf not implemented!").
+					Reason(collect.InvalidAnnotation).Build()
+				ctx.EXPECT().SetCondition(condition)
+				ctx.EXPECT().Error(errors.New("Annotation service.binding/foo: path={.status.foo},objectType=asdf not implemented!"))
+				ctx.EXPECT().StopProcessing()
+
+				collect.BindingDefinitions(ctx)
+			})
+		})
+
 		Context("on returning unexpected data", func() {
 			var (
 				service *mocks.MockService
@@ -752,7 +797,7 @@ var _ = Describe("Collect From Provisioned Service", func() {
 			err := errors.New("CRD of service ns1/foo indicates provisioned service, but no secret name provided under .status.binding.name")
 			crd := mocks.NewMockCRD(mockCtrl)
 			u := &unstructured.Unstructured{}
-			u.SetAnnotations(map[string]string{collect.ProvisionedServiceAnnotationKey: "true"})
+			u.SetAnnotations(map[string]string{binding.ProvisionedServiceAnnotationKey: "true"})
 
 			crd.EXPECT().Resource().Return(u)
 
diff --git a/test/acceptance/features/bindAppToServiceAnnotations.feature b/test/acceptance/features/bindAppToServiceAnnotations.feature
index cef30efc..f43860b3 100644
--- a/test/acceptance/features/bindAppToServiceAnnotations.feature
+++ b/test/acceptance/features/bindAppToServiceAnnotations.feature
@@ -584,3 +584,126 @@ Feature: Bind an application to a service using annotations
         And Service Binding CollectionReady.reason is "ValueNotFound"
         And Service Binding CollectionReady.message is "Value for key webarrows_primary not found"
 
+    Scenario: Application cannot be bound to service containing invalid elementType annotation
+        Given Generic test application is running
+        And CustomResourceDefinition backends.stable.example.com is available
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: $scenario_id
+                annotations:
+                    service.binding/credentials: path={.spec.connections.dbCredentials},elementType=asdf
+            spec:
+                connections:
+                  - type: primary
+                    url: primary.example.com
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: $scenario_id
+            spec:
+                bindAsFiles: false
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: $scenario_id
+                application:
+                    name: $scenario_id
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then Service Binding CollectionReady.status is "False"
+        And Service Binding CollectionReady.reason is "InvalidAnnotation"
+        And Service Binding Ready.message is "Annotation service.binding/credentials: path={.spec.connections.dbCredentials},elementType=asdf not implemented!"
+
+    Scenario: Application cannot be bound to service containing invalid objectType annotation
+        Given Generic test application is running
+        And CustomResourceDefinition backends.stable.example.com is available
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: $scenario_id
+                annotations:
+                    service.binding/credentials: path={.spec.connections.dbCredentials},objectType=asdf,sourceKey=username
+            spec:
+                connections:
+                  - type: primary
+                    url: primary.example.com
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: $scenario_id
+            spec:
+                bindAsFiles: false
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: $scenario_id
+                application:
+                    name: $scenario_id
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then Service Binding CollectionReady.status is "False"
+        And Service Binding CollectionReady.reason is "InvalidAnnotation"
+        And Service Binding Ready.message is "Annotation service.binding/credentials: path={.spec.connections.dbCredentials},objectType=asdf,sourceKey=username not implemented!"
+
+    Scenario: Application cannot be bound to service containing invalid path annotation
+        Given Generic test application is running
+        And CustomResourceDefinition backends.stable.example.com is available
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: $scenario_id
+                annotations:
+                    service.binding/credentials: path=asdf
+            spec:
+                connections:
+                  - type: primary
+                    url: primary.example.com
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: $scenario_id
+            spec:
+                bindAsFiles: false
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: $scenario_id
+                application:
+                    name: $scenario_id
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then Service Binding CollectionReady.status is "False"
+        And Service Binding CollectionReady.reason is "InvalidAnnotation"
+        And Service Binding CollectionReady.message is "Failed to create binding definition from "service.binding/credentials: path=asdf": could not create binding model for annotation key service.binding/credentials and value path=asdf: path has invalid syntax: "asdf""
+        And Service Binding Ready.message is "could not create binding model for annotation key service.binding/credentials and value path=asdf: path has invalid syntax: "asdf""