Skip to content
This repository was archived by the owner on Jun 26, 2024. It is now read-only.

Commit 2c4d36b

Browse files
sadlerapsadlerap
authored
Throw errors when elementType/objectType is wrong (#1023)
In a service binding annotation on a service, elementType and objectType could be garbage (i.e. elementType=asdf), and SBO would panic. We should instead do the following: 1. Throw an error instead of panicking when either of these two are invalid. 2. Fail the binding if those errors are thrown. Signed-off-by: Andy Sadler <[email protected]>
1 parent 4944c7b commit 2c4d36b

File tree

5 files changed

+244
-32
lines changed

5 files changed

+244
-32
lines changed

‎pkg/binding/annotationmapper.go

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ package binding
22

33
import (
44
"fmt"
5-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
65
"strings"
76

7+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
8+
89
"github.com/pkg/errors"
910
)
1011

@@ -26,12 +27,13 @@ var _ DefinitionBuilder = (*annotationBackedDefinitionBuilder)(nil)
2627
type modelKey string
2728

2829
const (
29-
pathModelKey modelKey = "path"
30-
objectTypeModelKey modelKey = "objectType"
31-
sourceKeyModelKey modelKey = "sourceKey"
32-
sourceValueModelKey modelKey = "sourceValue"
33-
elementTypeModelKey modelKey = "elementType"
34-
AnnotationPrefix = "service.binding"
30+
pathModelKey modelKey = "path"
31+
objectTypeModelKey modelKey = "objectType"
32+
sourceKeyModelKey modelKey = "sourceKey"
33+
sourceValueModelKey modelKey = "sourceValue"
34+
elementTypeModelKey modelKey = "elementType"
35+
AnnotationPrefix = "service.binding"
36+
ProvisionedServiceAnnotationKey = "servicebinding.io/provisioned-service"
3537
)
3638

3739
func NewDefinitionBuilder(annotationName string, annotationValue string, configMapReader UnstructuredResourceReader, secretReader UnstructuredResourceReader) *annotationBackedDefinitionBuilder {
@@ -45,26 +47,44 @@ func NewDefinitionBuilder(annotationName string, annotationValue string, configM
4547
}
4648
}
4749

48-
func (m *annotationBackedDefinitionBuilder) outputName() (string, error) {
49-
// bail out in the case the annotation name doesn't start with "service.binding"
50-
if m.name != AnnotationPrefix && !strings.HasPrefix(m.name, AnnotationPrefix+"/") {
51-
return "", fmt.Errorf("can't process annotation with name %q", m.name)
50+
func (m *annotationBackedDefinitionBuilder) isServiceBindingAnnotation() (bool, error) {
51+
if m.name == ProvisionedServiceAnnotationKey {
52+
return false, nil
53+
}
54+
55+
if m.name == AnnotationPrefix {
56+
return true, nil
57+
} else if strings.HasPrefix(m.name, AnnotationPrefix) {
58+
if strings.HasPrefix(m.name, AnnotationPrefix+"/") {
59+
return true, nil
60+
}
61+
62+
// it starts with AnnotationPrefix, but has extra text at the end not
63+
// separated by a /, so treat it as an error
64+
return false, fmt.Errorf("can't process annotation with name %q", m.name)
5265
}
5366

54-
if p := strings.SplitN(m.name, "/", 2); len(p) > 1 && len(p[1]) > 0 {
55-
return p[1], nil
67+
// bail out when the annotation name doesn't start with "service.binding"
68+
return false, nil
69+
}
70+
71+
func (m *annotationBackedDefinitionBuilder) outputName() string {
72+
73+
if p := strings.SplitN(m.name, "/", 2); len(p) == 2 && len(p[1]) > 0 {
74+
return p[1]
5675
}
5776

58-
return "", nil
77+
return ""
5978
}
6079

6180
func (m *annotationBackedDefinitionBuilder) Build() (Definition, error) {
6281

63-
outputName, err := m.outputName()
64-
if err != nil {
82+
if valid, err := m.isServiceBindingAnnotation(); !valid || err != nil {
6583
return nil, err
6684
}
6785

86+
outputName := m.outputName()
87+
6888
mod, err := newModel(m.value)
6989
if err != nil {
7090
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) {
128148
sourceValue: mod.sourceValue,
129149
}, nil
130150
}
131-
132-
panic(fmt.Sprintf("Annotation %s=%s not implemented!", m.name, m.value))
151+
return nil, fmt.Errorf("Annotation %s: %s not implemented!", m.name, m.value)
133152
}

‎pkg/binding/annotationmapper_test.go

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,17 @@ func TestAnnotationBackedBuilderInvalidAnnotation(t *testing.T) {
4141
},
4242
},
4343
{
44-
description: "other prefix supplied",
44+
description: "invalid element type",
4545
builder: &annotationBackedDefinitionBuilder{
46-
name: "other.prefix",
46+
name: "service.binding/databaseField",
47+
value: "path={.status.dbCredential},elementType=asdf",
48+
},
49+
},
50+
{
51+
description: "invalid object type",
52+
builder: &annotationBackedDefinitionBuilder{
53+
name: "service.binding/username",
54+
value: "path={.status.dbCredential},objectType=asdf,valueKey=username",
4755
},
4856
},
4957
}
@@ -77,7 +85,22 @@ func TestAnnotationBackedBuilderValidAnnotations(t *testing.T) {
7785
},
7886
},
7987
},
80-
88+
{
89+
description: "Ignore non-service binding annotations",
90+
builder: &annotationBackedDefinitionBuilder{
91+
name: "foo",
92+
value: "bar",
93+
},
94+
expectedValue: nil,
95+
},
96+
{
97+
description: "Ignore provisioned service binding annotations",
98+
builder: &annotationBackedDefinitionBuilder{
99+
name: ProvisionedServiceAnnotationKey,
100+
value: "true",
101+
},
102+
expectedValue: nil,
103+
},
81104
{
82105
description: "string definition",
83106
builder: &annotationBackedDefinitionBuilder{

‎pkg/reconcile/pipeline/handler/collect/impl.go

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ const (
2727
ErrorReadingBindingReason = "ErrorReadingBinding"
2828
ErrorReadingSecret = "ErrorReadingSecret"
2929

30-
ValueNotFound = "ValueNotFound"
30+
ValueNotFound = "ValueNotFound"
31+
InvalidAnnotation = "InvalidAnnotation"
3132
)
3233

3334
func PreFlight(ctx pipeline.Context) {
@@ -65,9 +66,14 @@ func BindingDefinitions(ctx pipeline.Context) {
6566
for k, v := range anns {
6667
definition, err := makeBindingDefinition(k, v, ctx)
6768
if err != nil {
68-
continue
69+
condition := notCollectionReadyCond(InvalidAnnotation, fmt.Errorf("Failed to create binding definition from \"%v: %v\": %v", k, v, err))
70+
ctx.SetCondition(condition)
71+
ctx.Error(err)
72+
ctx.StopProcessing()
73+
}
74+
if definition != nil {
75+
service.AddBindingDef(definition)
6976
}
70-
service.AddBindingDef(definition)
7177
}
7278
}
7379
}
@@ -96,8 +102,6 @@ func BindingItems(ctx pipeline.Context) {
96102
}
97103
}
98104

99-
const ProvisionedServiceAnnotationKey = "servicebinding.io/provisioned-service"
100-
101105
func ProvisionedService(ctx pipeline.Context) {
102106
services, _ := ctx.Services()
103107

@@ -126,7 +130,7 @@ func ProvisionedService(ctx pipeline.Context) {
126130
if crd == nil {
127131
continue
128132
}
129-
v, ok := crd.Resource().GetAnnotations()[ProvisionedServiceAnnotationKey]
133+
v, ok := crd.Resource().GetAnnotations()[binding.ProvisionedServiceAnnotationKey]
130134
if ok && v == "true" {
131135
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()))
132136
return
@@ -255,9 +259,7 @@ func collectItems(prefix string, ctx pipeline.Context, service pipeline.Service,
255259
if mapVal := v.MapIndex(n).Interface(); mapVal != nil {
256260
collectItems(p, ctx, service, n, mapVal)
257261
} else {
258-
condition := apis.Conditions().NotCollectionReady().
259-
Msg(fmt.Sprintf("Value for key %v_%v not found", prefix+k.String(), n.String())).
260-
Reason(ValueNotFound).Build()
262+
condition := notCollectionReadyCond(ValueNotFound, fmt.Errorf("Value for key %v_%v not found", prefix+k.String(), n.String()))
261263
ctx.SetCondition(condition)
262264
ctx.Error(ErrorValueNotFound)
263265
ctx.StopProcessing()

‎pkg/reconcile/pipeline/handler/collect/impl_test.go

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ var _ = Describe("Collect Binding Definitions", func() {
156156

157157
serviceContent.SetAnnotations(map[string]string{
158158
"foo": "bar",
159-
collect.ProvisionedServiceAnnotationKey: "true",
159+
binding.ProvisionedServiceAnnotationKey: "true",
160160
"service.binding/foo": "path={.status.foo},objectType=Secret,sourceValue=username",
161161
"service.binding/foo2": "path={.status.foo2},objectType=Secret,sourceValue=username",
162162
})
@@ -420,6 +420,51 @@ var _ = Describe("Collect Binding Data", func() {
420420
)
421421
})
422422

423+
Context("on invalid annotations", func() {
424+
var (
425+
service *mocks.MockService
426+
serviceContent *unstructured.Unstructured
427+
)
428+
BeforeEach(func() {
429+
service = mocks.NewMockService(mockCtrl)
430+
serviceContent = &unstructured.Unstructured{}
431+
service.EXPECT().CustomResourceDefinition().Return(nil, nil)
432+
service.EXPECT().Resource().Return(serviceContent)
433+
434+
ctx.EXPECT().Services().Return([]pipeline.Service{service}, nil)
435+
})
436+
437+
It("should error when elementType is invalid", func() {
438+
serviceContent.SetAnnotations(map[string]string{
439+
"service.binding/foo": "path={.status.foo},elementType=asdf",
440+
})
441+
442+
condition := apis.Conditions().NotCollectionReady().
443+
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!").
444+
Reason(collect.InvalidAnnotation).Build()
445+
ctx.EXPECT().SetCondition(condition)
446+
ctx.EXPECT().Error(errors.New("Annotation service.binding/foo: path={.status.foo},elementType=asdf not implemented!"))
447+
ctx.EXPECT().StopProcessing()
448+
449+
collect.BindingDefinitions(ctx)
450+
})
451+
452+
It("should error when objectType is invalid", func() {
453+
serviceContent.SetAnnotations(map[string]string{
454+
"service.binding/foo": "path={.status.foo},objectType=asdf",
455+
})
456+
457+
condition := apis.Conditions().NotCollectionReady().
458+
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!").
459+
Reason(collect.InvalidAnnotation).Build()
460+
ctx.EXPECT().SetCondition(condition)
461+
ctx.EXPECT().Error(errors.New("Annotation service.binding/foo: path={.status.foo},objectType=asdf not implemented!"))
462+
ctx.EXPECT().StopProcessing()
463+
464+
collect.BindingDefinitions(ctx)
465+
})
466+
})
467+
423468
Context("on returning unexpected data", func() {
424469
var (
425470
service *mocks.MockService
@@ -752,7 +797,7 @@ var _ = Describe("Collect From Provisioned Service", func() {
752797
err := errors.New("CRD of service ns1/foo indicates provisioned service, but no secret name provided under .status.binding.name")
753798
crd := mocks.NewMockCRD(mockCtrl)
754799
u := &unstructured.Unstructured{}
755-
u.SetAnnotations(map[string]string{collect.ProvisionedServiceAnnotationKey: "true"})
800+
u.SetAnnotations(map[string]string{binding.ProvisionedServiceAnnotationKey: "true"})
756801

757802
crd.EXPECT().Resource().Return(u)
758803

‎test/acceptance/features/bindAppToServiceAnnotations.feature

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -584,3 +584,126 @@ Feature: Bind an application to a service using annotations
584584
And Service Binding CollectionReady.reason is "ValueNotFound"
585585
And Service Binding CollectionReady.message is "Value for key webarrows_primary not found"
586586

587+
Scenario: Application cannot be bound to service containing invalid elementType annotation
588+
Given Generic test application is running
589+
And CustomResourceDefinition backends.stable.example.com is available
590+
And The Custom Resource is present
591+
"""
592+
apiVersion: stable.example.com/v1
593+
kind: Backend
594+
metadata:
595+
name: $scenario_id
596+
annotations:
597+
service.binding/credentials: path={.spec.connections.dbCredentials},elementType=asdf
598+
spec:
599+
connections:
600+
- type: primary
601+
url: primary.example.com
602+
status:
603+
somestatus: good
604+
"""
605+
When Service Binding is applied
606+
"""
607+
apiVersion: binding.operators.coreos.com/v1alpha1
608+
kind: ServiceBinding
609+
metadata:
610+
name: $scenario_id
611+
spec:
612+
bindAsFiles: false
613+
services:
614+
- group: stable.example.com
615+
version: v1
616+
kind: Backend
617+
name: $scenario_id
618+
application:
619+
name: $scenario_id
620+
group: apps
621+
version: v1
622+
resource: deployments
623+
"""
624+
Then Service Binding CollectionReady.status is "False"
625+
And Service Binding CollectionReady.reason is "InvalidAnnotation"
626+
And Service Binding Ready.message is "Annotation service.binding/credentials: path={.spec.connections.dbCredentials},elementType=asdf not implemented!"
627+
628+
Scenario: Application cannot be bound to service containing invalid objectType annotation
629+
Given Generic test application is running
630+
And CustomResourceDefinition backends.stable.example.com is available
631+
And The Custom Resource is present
632+
"""
633+
apiVersion: stable.example.com/v1
634+
kind: Backend
635+
metadata:
636+
name: $scenario_id
637+
annotations:
638+
service.binding/credentials: path={.spec.connections.dbCredentials},objectType=asdf,sourceKey=username
639+
spec:
640+
connections:
641+
- type: primary
642+
url: primary.example.com
643+
status:
644+
somestatus: good
645+
"""
646+
When Service Binding is applied
647+
"""
648+
apiVersion: binding.operators.coreos.com/v1alpha1
649+
kind: ServiceBinding
650+
metadata:
651+
name: $scenario_id
652+
spec:
653+
bindAsFiles: false
654+
services:
655+
- group: stable.example.com
656+
version: v1
657+
kind: Backend
658+
name: $scenario_id
659+
application:
660+
name: $scenario_id
661+
group: apps
662+
version: v1
663+
resource: deployments
664+
"""
665+
Then Service Binding CollectionReady.status is "False"
666+
And Service Binding CollectionReady.reason is "InvalidAnnotation"
667+
And Service Binding Ready.message is "Annotation service.binding/credentials: path={.spec.connections.dbCredentials},objectType=asdf,sourceKey=username not implemented!"
668+
669+
Scenario: Application cannot be bound to service containing invalid path annotation
670+
Given Generic test application is running
671+
And CustomResourceDefinition backends.stable.example.com is available
672+
And The Custom Resource is present
673+
"""
674+
apiVersion: stable.example.com/v1
675+
kind: Backend
676+
metadata:
677+
name: $scenario_id
678+
annotations:
679+
service.binding/credentials: path=asdf
680+
spec:
681+
connections:
682+
- type: primary
683+
url: primary.example.com
684+
status:
685+
somestatus: good
686+
"""
687+
When Service Binding is applied
688+
"""
689+
apiVersion: binding.operators.coreos.com/v1alpha1
690+
kind: ServiceBinding
691+
metadata:
692+
name: $scenario_id
693+
spec:
694+
bindAsFiles: false
695+
services:
696+
- group: stable.example.com
697+
version: v1
698+
kind: Backend
699+
name: $scenario_id
700+
application:
701+
name: $scenario_id
702+
group: apps
703+
version: v1
704+
resource: deployments
705+
"""
706+
Then Service Binding CollectionReady.status is "False"
707+
And Service Binding CollectionReady.reason is "InvalidAnnotation"
708+
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""
709+
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""

0 commit comments

Comments
 (0)