From 7a9608220dc0d0383ba3df2aaf2db88e7f340c19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ota=CC=81vio=20Fernandes?= Date: Mon, 16 Dec 2019 08:16:29 +0100 Subject: [PATCH 01/29] Improving .editorconfig file. --- .editorconfig | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.editorconfig b/.editorconfig index 55f806080c..48c1ed8542 100644 --- a/.editorconfig +++ b/.editorconfig @@ -12,7 +12,13 @@ indent_size = 4 max_line_length = 101 -[*.yml,*.yaml,*.toml] +[*.yml] +indent_size = 2 + +[*.yaml] +indent_size = 2 + +[*.toml] indent_size = 2 [*.md] From 38c90149b8498fe3c398156c5b4935c675a3e396 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ota=CC=81vio=20Fernandes?= Date: Mon, 16 Dec 2019 08:16:47 +0100 Subject: [PATCH 02/29] BackingServiceSelectors. --- ...enshift.io_servicebindingrequests_crd.yaml | 232 ++++++++++++++++++ .../v1alpha1/servicebindingrequest_types.go | 3 + .../apps/v1alpha1/zz_generated.deepcopy.go | 5 + .../apps/v1alpha1/zz_generated.openapi.go | 15 +- 4 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml diff --git a/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml b/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml new file mode 100644 index 0000000000..5219df3ce9 --- /dev/null +++ b/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml @@ -0,0 +1,232 @@ +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: servicebindingrequests.apps.openshift.io +spec: + group: apps.openshift.io + names: + kind: ServiceBindingRequest + listKind: ServiceBindingRequestList + plural: servicebindingrequests + shortNames: + - sbr + - sbrs + singular: servicebindingrequest + scope: Namespaced + subresources: + status: {} + validation: + openAPIV3Schema: + description: ServiceBindingRequest is the Schema for the servicebindings API + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: ServiceBindingRequestSpec defines the desired state of ServiceBindingRequest + properties: + applicationSelector: + description: ApplicationSelector is used to identify the application + connecting to the backing service operator. + properties: + group: + type: string + matchLabels: + additionalProperties: + type: string + type: object + resource: + type: string + resourceRef: + type: string + version: + type: string + required: + - matchLabels + - resource + - resourceRef + - version + type: object + backingServiceSelector: + description: BackingServiceSelector is used to identify the backing + service operator. + properties: + group: + type: string + kind: + type: string + resourceRef: + type: string + version: + type: string + required: + - group + - kind + - resourceRef + - version + type: object + backingServiceSelectors: + description: BackingServiceSelectors is an slice of BackingServiceSelector + items: + description: BackingServiceSelector defines the selector based on + resource name, version, and resource kind + properties: + group: + type: string + kind: + type: string + resourceRef: + type: string + version: + type: string + required: + - group + - kind + - resourceRef + - version + type: object + type: array + customEnvVar: + description: Custom env variables + items: + description: EnvVar represents an environment variable present in + a Container. + properties: + name: + description: Name of the environment variable. Must be a C_IDENTIFIER. + type: string + value: + description: 'Variable references $(VAR_NAME) are expanded using + the previous defined environment variables in the container + and any service environment variables. If a variable cannot + be resolved, the reference in the input string will be unchanged. + The $(VAR_NAME) syntax can be escaped with a double $$, ie: + $$(VAR_NAME). Escaped references will never be expanded, regardless + of whether the variable exists or not. Defaults to "".' + type: string + valueFrom: + description: Source for the environment variable's value. Cannot + be used if value is not empty. + properties: + configMapKeyRef: + description: Selects a key of a ConfigMap. + properties: + key: + description: The key to select. + type: string + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + optional: + description: Specify whether the ConfigMap or its key + must be defined + type: boolean + required: + - key + type: object + fieldRef: + description: 'Selects a field of the pod: supports metadata.name, + metadata.namespace, metadata.labels, metadata.annotations, + spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP.' + properties: + apiVersion: + description: Version of the schema the FieldPath is written + in terms of, defaults to "v1". + type: string + fieldPath: + description: Path of the field to select in the specified + API version. + type: string + required: + - fieldPath + type: object + resourceFieldRef: + description: 'Selects a resource of the container: only resources + limits and requests (limits.cpu, limits.memory, limits.ephemeral-storage, + requests.cpu, requests.memory and requests.ephemeral-storage) + are currently supported.' + properties: + containerName: + description: 'Container name: required for volumes, optional + for env vars' + type: string + divisor: + description: Specifies the output format of the exposed + resources, defaults to "1" + type: string + resource: + description: 'Required: resource to select' + type: string + required: + - resource + type: object + secretKeyRef: + description: Selects a key of a secret in the pod's namespace + properties: + key: + description: The key of the secret to select from. Must + be a valid secret key. + type: string + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + optional: + description: Specify whether the Secret or its key must + be defined + type: boolean + required: + - key + type: object + type: object + required: + - name + type: object + type: array + detectBindingResources: + description: DetectBindingResources is flag used to bind all non-bindable + variables from different subresources owned by backing operator CR. + type: boolean + envVarPrefix: + description: EnvVarPrefix is the prefix for environment variables + type: string + mountPathPrefix: + description: MountPathPrefix is the prefix for volume mount + type: string + required: + - applicationSelector + - backingServiceSelector + - backingServiceSelectors + type: object + status: + description: ServiceBindingRequestStatus defines the observed state of ServiceBindingRequest + properties: + applicationObjects: + description: ApplicationObjects contains all the application objects + filtered by label + items: + type: string + type: array + bindingStatus: + description: BindingStatus is the status of the service binding request. + type: string + secret: + description: Secret is the name of the intermediate secret + type: string + type: object + type: object + version: v1alpha1 + versions: + - name: v1alpha1 + served: true + storage: true diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go index c6c116431b..c80f2c7686 100644 --- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go +++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go @@ -28,6 +28,9 @@ type ServiceBindingRequestSpec struct { // BackingServiceSelector is used to identify the backing service operator. BackingServiceSelector BackingServiceSelector `json:"backingServiceSelector"` + // BackingServiceSelectors is an slice of BackingServiceSelector + BackingServiceSelectors []BackingServiceSelector `json:"backingServiceSelectors"` + // ApplicationSelector is used to identify the application connecting to the // backing service operator. ApplicationSelector ApplicationSelector `json:"applicationSelector"` diff --git a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go index 8086e9d2b4..9b2f03bee3 100644 --- a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go @@ -120,6 +120,11 @@ func (in *ServiceBindingRequestSpec) DeepCopyInto(out *ServiceBindingRequestSpec } } out.BackingServiceSelector = in.BackingServiceSelector + if in.BackingServiceSelectors != nil { + in, out := &in.BackingServiceSelectors, &out.BackingServiceSelectors + *out = make([]BackingServiceSelector, len(*in)) + copy(*out, *in) + } in.ApplicationSelector.DeepCopyInto(&out.ApplicationSelector) return } diff --git a/pkg/apis/apps/v1alpha1/zz_generated.openapi.go b/pkg/apis/apps/v1alpha1/zz_generated.openapi.go index 19d74f4279..65ae3ea5f3 100644 --- a/pkg/apis/apps/v1alpha1/zz_generated.openapi.go +++ b/pkg/apis/apps/v1alpha1/zz_generated.openapi.go @@ -193,6 +193,19 @@ func schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestSpec(ref common.Referenc Ref: ref("github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.BackingServiceSelector"), }, }, + "backingServiceSelectors": { + SchemaProps: spec.SchemaProps{ + Description: "BackingServiceSelectors is an slice of BackingServiceSelector", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.BackingServiceSelector"), + }, + }, + }, + }, + }, "applicationSelector": { SchemaProps: spec.SchemaProps{ Description: "ApplicationSelector is used to identify the application connecting to the backing service operator.", @@ -207,7 +220,7 @@ func schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestSpec(ref common.Referenc }, }, }, - Required: []string{"backingServiceSelector", "applicationSelector"}, + Required: []string{"backingServiceSelector", "backingServiceSelectors", "applicationSelector"}, }, }, Dependencies: []string{ From 3eb18cb61924ef69a1c0b2e5f5654ce22da2a5cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ota=CC=81vio=20Fernandes?= Date: Mon, 16 Dec 2019 08:37:15 +0100 Subject: [PATCH 03/29] Refactoring SBR validation. --- .../servicebindingrequest/reconciler.go | 39 ++++++++----------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index 3b35909e9c..84bf0ceddf 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -2,7 +2,6 @@ package servicebindingrequest import ( "context" - "errors" "fmt" "gotest.tools/assert/cmp" @@ -67,6 +66,16 @@ func (r *Reconciler) setApplicationObjects( sbrStatus.ApplicationObjects = names } +// validateServiceBindingRequest check for unsupported settings in SBR. +func (r *Reconciler) validateServiceBindingRequest(sbr *v1alpha1.ServiceBindingRequest) error { + // check if application ResourceRef and MatchLabels, one of them is required. + if sbr.Spec.ApplicationSelector.ResourceRef == "" && + sbr.Spec.ApplicationSelector.MatchLabels == nil { + return fmt.Errorf("both ResourceRef and MatchLabels are not set") + } + return nil +} + // getServiceBindingRequest retrieve the SBR object based on namespaced-name. func (r *Reconciler) getServiceBindingRequest( namespacedName types.NamespacedName, @@ -77,11 +86,16 @@ func (r *Reconciler) getServiceBindingRequest( if err != nil { return nil, err } + sbr := &v1alpha1.ServiceBindingRequest{} err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) if err != nil { return nil, err } + + if err = r.validateServiceBindingRequest(sbr); err != nil { + return nil, err + } return sbr, nil } @@ -162,22 +176,6 @@ func (r *Reconciler) onError( return RequeueOnNotFound(err, requeueAfter) } -// checkSBR checks the Service Binding Request -func checkSBR(sbr *v1alpha1.ServiceBindingRequest, log *log.Log) error { - // Check if application ResourceRef is present - if sbr.Spec.ApplicationSelector.ResourceRef == "" { - log.Debug("Spec.ApplicationSelector.ResourceRef not found") - - // Check if MatchLabels is present - if sbr.Spec.ApplicationSelector.MatchLabels == nil { - err := errors.New("NotFoundError") - log.Error(err, "Spec.ApplicationSelector.MatchLabels not found") - return err - } - } - return nil -} - // unbind removes the relationship between the given sbr and the manifests the operator has // previously modified. This process also deletes any manifests created to support the binding // functionality, such as ConfigMaps and Secrets. @@ -313,7 +311,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err logger.Info("Reconciling ServiceBindingRequest...") - // fetch the ServiceBindingRequest instance + // fetch and validate namespaced ServiceBindingRequest instance sbr, err := r.getServiceBindingRequest(request.NamespacedName) if err != nil { logger.Error(err, "On retrieving service-binding-request instance.") @@ -326,11 +324,6 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // splitting instance from it's status sbrStatus := &sbr.Status - // check Service Binding Request - if err = checkSBR(sbr, logger); err != nil { - return RequeueError(err) - } - // // Planing changes // From 3fcc2a787ee2e359faab586b71c3260ccc9e5de9 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 18 Dec 2019 11:38:45 +0100 Subject: [PATCH 04/29] Embed GroupVersionKind in BackingServiceSelector and other cleanups --- .../v1alpha1/servicebindingrequest_types.go | 7 +- .../apps/v1alpha1/zz_generated.deepcopy.go | 1 + .../servicebindingrequest/planner.go | 77 ++++++++----------- .../servicebindingrequest/planner_test.go | 5 +- .../sbrcontroller_test.go | 17 ++-- test/mocks/fake.go | 9 ++- test/mocks/mocks.go | 15 ++-- 7 files changed, 63 insertions(+), 68 deletions(-) diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go index c80f2c7686..6c800f4fdd 100644 --- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go +++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go @@ -3,6 +3,7 @@ package v1alpha1 import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" ) // Important: Run "operator-sdk generate k8s" to regenerate code after modifying this file @@ -55,10 +56,8 @@ type ServiceBindingRequestStatus struct { // BackingServiceSelector defines the selector based on resource name, version, and resource kind // +k8s:openapi-gen=true type BackingServiceSelector struct { - Group string `json:"group"` - Version string `json:"version"` - Kind string `json:"kind"` - ResourceRef string `json:"resourceRef"` + schema.GroupVersionKind `json:",inline"` + ResourceRef string `json:"resourceRef"` } // ApplicationSelector defines the selector based on labels and GVR diff --git a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go index 9b2f03bee3..71d4b7f778 100644 --- a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go @@ -35,6 +35,7 @@ func (in *ApplicationSelector) DeepCopy() *ApplicationSelector { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BackingServiceSelector) DeepCopyInto(out *BackingServiceSelector) { *out = *in + out.GroupVersionKind = in.GroupVersionKind return } diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go index 3766cfc165..e0b21313db 100644 --- a/pkg/controller/servicebindingrequest/planner.go +++ b/pkg/controller/servicebindingrequest/planner.go @@ -2,7 +2,6 @@ package servicebindingrequest import ( "context" - "strings" olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" "k8s.io/apimachinery/pkg/api/meta" @@ -38,71 +37,55 @@ type Plan struct { } // searchCR based on a CustomResourceDefinitionDescription and name, search for the object. -func (p *Planner) searchCR() (*unstructured.Unstructured, error) { - bss := p.sbr.Spec.BackingServiceSelector - gvk := schema.GroupVersionKind{Group: bss.Group, Version: bss.Version, Kind: bss.Kind} - gvr, _ := meta.UnsafeGuessKindToResource(gvk) - opts := metav1.GetOptions{} - - log := p.logger.WithValues("CR.GVK", gvk.String(), "CR.GVR", gvr.String()) - log.Debug("Searching for CR instance...") - - cr, err := p.client.Resource(gvr).Namespace(p.sbr.GetNamespace()).Get(bss.ResourceRef, opts) - - if err != nil { - log.Info("during reading CR") - return nil, err - } +func (p *Planner) searchCR(namespace string, selector v1alpha1.BackingServiceSelector) (*unstructured.Unstructured, error) { + // gvr is the plural guessed resource for the given selector + gvr, _ := meta.UnsafeGuessKindToResource(selector.GroupVersionKind) + // delegate the search selector's namespaced resource client + return p.client.Resource(gvr).Namespace(namespace).Get(selector.ResourceRef, metav1.GetOptions{}) +} - log.Debug("Found target CR!", "CR.Name", cr.GetName()) - return cr, nil +// CRDGVR is the plural GVR for Kubernetes CRDs. +var CRDGVR = schema.GroupVersionResource{ + Group: "apiextensions.k8s.io", + Version: "v1beta1", + Resource: "customresourcedefinitions", } -// searchCRD based on a CustomResourceDefinitionDescription and name, search for the object. -func (p *Planner) searchCRD() (*unstructured.Unstructured, error) { - bss := p.sbr.Spec.BackingServiceSelector - gvk := schema.GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1beta1", Kind: "CustomResourceDefinition"} +// searchCRD returns the CRD related to the gvk. +func (p *Planner) searchCRD(gvk schema.GroupVersionKind) (*unstructured.Unstructured, error) { + // gvr is the plural guessed resource for the given GVK gvr, _ := meta.UnsafeGuessKindToResource(gvk) - opts := metav1.GetOptions{} - - logger := p.logger.WithValues("CR.GVK", gvk.String(), "CR.GVR", gvr.String(), "Kind", bss.Kind) - logger.Info("Searching for CRD instance...") - - // TODO: This hack should be removed! Probably the name should be prompted from user through SBR CR. - name := strings.ToLower(bss.Kind) + "s." + bss.Group - crd, err := p.client.Resource(gvr).Get(name, opts) - - if err != nil { - logger.Info("during reading CRD") - return nil, err - } - - logger.WithValues("CR.Name", crd.GetName()).Info("Found target CR!") - return crd, nil + // crdName is the string'fied GroupResource, e.g. "deployments.apps" + crdName := gvr.GroupResource().String() + // delegate the search to the CustomResourceDefinition resource client + return p.client.Resource(CRDGVR).Get(crdName, metav1.GetOptions{}) } // Plan by retrieving the necessary resources related to binding a service backend. func (p *Planner) Plan() (*Plan, error) { - bss := p.sbr.Spec.BackingServiceSelector - gvk := schema.GroupVersionKind{Group: bss.Group, Version: bss.Version, Kind: bss.Kind} - olm := NewOLM(p.client, p.sbr.GetNamespace()) - crd, err := p.searchCRD() + bssGVK := p.sbr.Spec.BackingServiceSelector.GroupVersionKind + + // resolve the CRD using the service's GVK + crd, err := p.searchCRD(bssGVK) if err != nil { return nil, err } + p.logger.Debug("Resolved CRD", "CRD", crd) - p.logger.Debug("After search crd", "CRD", crd) - - crdDescription, err := olm.SelectCRDByGVK(gvk, crd) + // resolve the CRDDescription based on the service's GVK and the resolved CRD + olm := NewOLM(p.client, p.sbr.GetNamespace()) + crdDescription, err := olm.SelectCRDByGVK(bssGVK, crd) if err != nil { return nil, err } + p.logger.Debug("Resolved CRDDescription", "CRDDescription", crdDescription) - // retrieve the CR based on kind, api-version and name - cr, err := p.searchCR() + // retrieve the CR referred by the service + cr, err := p.searchCR(p.sbr.GetNamespace(), p.sbr.Spec.BackingServiceSelector) if err != nil { return nil, err } + p.logger.Debug("Resolved CR", "CR", cr) return &Plan{ Ns: p.sbr.GetNamespace(), diff --git a/pkg/controller/servicebindingrequest/planner_test.go b/pkg/controller/servicebindingrequest/planner_test.go index cdc0c21f31..d47934d0c2 100644 --- a/pkg/controller/servicebindingrequest/planner_test.go +++ b/pkg/controller/servicebindingrequest/planner_test.go @@ -34,7 +34,7 @@ func TestPlannerNew(t *testing.T) { require.NotNil(t, planner) t.Run("searchCR", func(t *testing.T) { - cr, err := planner.searchCR() + cr, err := planner.searchCR(ns, sbr.Spec.BackingServiceSelector) require.NoError(t, err) require.NotNil(t, cr) @@ -63,12 +63,13 @@ func TestPlannerAnnotation(t *testing.T) { f := mocks.NewFake(t, ns) sbr := f.AddMockedServiceBindingRequest(name, resourceRef, "", deploymentsGVR, matchLabels) f.AddMockedUnstructuredDatabaseCRD() + cr := f.AddMockedDatabaseCR("database") planner = NewPlanner(context.TODO(), f.FakeDynClient(), sbr) require.NotNil(t, planner) t.Run("searchCRD", func(t *testing.T) { - crd, err := planner.searchCRD() + crd, err := planner.searchCRD(cr.GetObjectKind().GroupVersionKind()) require.NoError(t, err) require.NotNil(t, crd) diff --git a/pkg/controller/servicebindingrequest/sbrcontroller_test.go b/pkg/controller/servicebindingrequest/sbrcontroller_test.go index 99ec7b6c6b..2e54d69033 100644 --- a/pkg/controller/servicebindingrequest/sbrcontroller_test.go +++ b/pkg/controller/servicebindingrequest/sbrcontroller_test.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/event" "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" @@ -29,9 +30,11 @@ func TestSBRControllerBuildSBRPredicate(t *testing.T) { sbrA := &v1alpha1.ServiceBindingRequest{ Spec: v1alpha1.ServiceBindingRequestSpec{ BackingServiceSelector: v1alpha1.BackingServiceSelector{ - Group: "test", - Version: "v1alpha1", - Kind: "TestHost", + GroupVersionKind: schema.GroupVersionKind{ + Group: "test", + Version: "v1alpha1", + Kind: "TestHost", + }, ResourceRef: "", }, }, @@ -39,9 +42,11 @@ func TestSBRControllerBuildSBRPredicate(t *testing.T) { sbrB := &v1alpha1.ServiceBindingRequest{ Spec: v1alpha1.ServiceBindingRequestSpec{ BackingServiceSelector: v1alpha1.BackingServiceSelector{ - Group: "test", - Version: "v1", - Kind: "TestHost", + GroupVersionKind: schema.GroupVersionKind{ + Group: "test", + Version: "v1", + Kind: "TestHost", + }, ResourceRef: "", }, }, diff --git a/test/mocks/fake.go b/test/mocks/fake.go index 9754bf92d2..3aea10a97c 100644 --- a/test/mocks/fake.go +++ b/test/mocks/fake.go @@ -106,10 +106,12 @@ func (f *Fake) AddMockedUnstructuredCSVWithVolumeMount(name string) { } // AddMockedDatabaseCR add mocked object from DatabaseCRMock. -func (f *Fake) AddMockedDatabaseCR(ref string) { +func (f *Fake) AddMockedDatabaseCR(ref string) runtime.Object { require.NoError(f.t, pgapis.AddToScheme(f.S)) f.S.AddKnownTypes(pgv1alpha1.SchemeGroupVersion, &pgv1alpha1.Database{}) - f.objs = append(f.objs, DatabaseCRMock(f.ns, ref)) + mock := DatabaseCRMock(f.ns, ref) + f.objs = append(f.objs, mock) + return mock } func (f *Fake) AddMockedUnstructuredDatabaseCR(ref string) { @@ -137,12 +139,13 @@ func (f *Fake) AddMockedUnstructuredDeployment(name string, matchLabels map[stri f.objs = append(f.objs, d) } -func (f *Fake) AddMockedUnstructuredDatabaseCRD() { +func (f *Fake) AddMockedUnstructuredDatabaseCRD() *unstructured.Unstructured { require.NoError(f.t, apiextensionv1beta1.AddToScheme(f.S)) c, err := UnstructuredDatabaseCRDMock(f.ns) require.NoError(f.t, err) f.S.AddKnownTypes(apiextensionv1beta1.SchemeGroupVersion, &apiextensionv1beta1.CustomResourceDefinition{}) f.objs = append(f.objs, c) + return c } func (f *Fake) AddMockedUnstructuredPostgresDatabaseCR(ref string) { diff --git a/test/mocks/mocks.go b/test/mocks/mocks.go index c9b4b61624..119eabe59a 100644 --- a/test/mocks/mocks.go +++ b/test/mocks/mocks.go @@ -10,8 +10,6 @@ import ( pgv1alpha1 "github.com/operator-backing-service-samples/postgresql-operator/pkg/apis/postgresql/v1alpha1" olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" olminstall "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install" - v1alpha1 "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" - "github.com/redhat-developer/service-binding-operator/pkg/converter" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apiextensionv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" @@ -20,6 +18,9 @@ import ( ustrv1 "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + + v1alpha1 "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" + "github.com/redhat-developer/service-binding-operator/pkg/converter" ) // resource details employed in mocks @@ -392,9 +393,11 @@ func ServiceBindingRequestMock( }, }, BackingServiceSelector: v1alpha1.BackingServiceSelector{ - Group: CRDName, - Version: CRDVersion, - Kind: CRDKind, + GroupVersionKind: schema.GroupVersionKind{ + Group: CRDName, + Version: CRDVersion, + Kind: CRDKind, + }, ResourceRef: backingServiceResourceRef, }, ApplicationSelector: v1alpha1.ApplicationSelector{ @@ -532,7 +535,7 @@ func DeploymentMock(ns, name string, matchLabels map[string]string) appsv1.Deplo } } -//ThirdLevel ... +// ThirdLevel ... type ThirdLevel struct { Something string `json:"something"` } From 953d7d075017fef3a6746a02cb74f4b6dc8278c6 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 19 Dec 2019 11:03:37 +0100 Subject: [PATCH 05/29] Refactor Reconciler and friends to simplify the code --- go.sum | 2 +- .../servicebindingrequest/reconciler.go | 161 +++++++++++------- .../servicebindingrequest/retriever.go | 68 +++++--- .../servicebindingrequest/retriever_test.go | 4 +- test/e2e/servicebindingrequest_test.go | 5 +- 5 files changed, 145 insertions(+), 95 deletions(-) diff --git a/go.sum b/go.sum index 7b8cbebaa4..cf78eed484 100644 --- a/go.sum +++ b/go.sum @@ -459,7 +459,7 @@ github.com/operator-framework/operator-registry v1.0.1/go.mod h1:1xEdZjjUg2hPEd5 github.com/operator-framework/operator-registry v1.0.4/go.mod h1:hve6YwcjM2nGVlscLtNsp9sIIBkNZo6jlJgzWw7vP9s= github.com/operator-framework/operator-registry v1.1.1/go.mod h1:7D4WEwL+EKti5npUh4/u64DQhawCBRugp8Ql20duUb4= github.com/operator-framework/operator-sdk v0.8.2-0.20190522220659-031d71ef8154/go.mod h1:iVyukRkam5JZa8AnjYf+/G3rk7JI1+M6GsU0sq0B9NA= -github.com/operator-framework/operator-sdk v0.12.0 h1:9eAD1L8e6pPCpFCAacBUVf2eloDkRuVm29GTCOktLqQ= +github.com/operator-framework/operator-sdk v0.12.0 h1:aD4qPbSAbZgRj1jFdFLq/dBI4P4aKX8d4rJowyQtTYM= github.com/operator-framework/operator-sdk v0.12.0/go.mod h1:mW8isQxiXlLCVf2E+xqflkQAVLOTbiqjndKdkKIrR0M= github.com/pborman/uuid v0.0.0-20180906182336-adf5a7427709/go.mod h1:VyrYX9gd7irzKovcSS6BIIEwPRkP2Wm2m9ufcdFSJ34= github.com/pborman/uuid v1.2.0 h1:J7Q5mO4ysT1dv8hyrUGHb9+ooztCXu1D8MY8DZYsu3g= diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index 84bf0ceddf..99fc96c397 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -8,6 +8,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "sigs.k8s.io/controller-runtime/pkg/client" @@ -179,33 +180,19 @@ func (r *Reconciler) onError( // unbind removes the relationship between the given sbr and the manifests the operator has // previously modified. This process also deletes any manifests created to support the binding // functionality, such as ConfigMaps and Secrets. -func (r *Reconciler) unbind( - logger *log.Log, - binder *Binder, - secret *Secret, - sbr *v1alpha1.ServiceBindingRequest, - objectsToAnnotate []*unstructured.Unstructured, -) (reconcile.Result, error) { +func (r *Reconciler) unbind(logger *log.Log, binder *Binder, bindingContext *BindingContext) (reconcile.Result, error) { + var err error + logger = logger.WithName("unbind") // when finalizer is not found anymore, it can be safely removed - if !containsStringSlice(sbr.GetFinalizers(), sbrFinalizer) { + if !containsStringSlice(bindingContext.SBR.GetFinalizers(), sbrFinalizer) { logger.Info("Resource can be safely deleted!") return Done() } - logger.Debug("Reading intermediary secret before deletion.") - secretObj, err := secret.Get() - if err != nil { - logger.Error(err, "On reading intermediary secret.") - return RequeueError(err) - } - - // adding secret on list of objects, to remove annotations from secret before deletion - objectsToAnnotate = append(objectsToAnnotate, secretObj) - logger.Info("Cleaning related objects from operator's annotations...") - if err = RemoveSBRAnnotations(r.dynClient, objectsToAnnotate); err != nil { + if err = RemoveSBRAnnotations(r.dynClient, bindingContext.Objects); err != nil { logger.Error(err, "On removing annotations from related objects.") return RequeueError(err) } @@ -218,14 +205,14 @@ func (r *Reconciler) unbind( } logger.Info("Deleting intermediary secret...") - if err = secret.Delete(); err != nil { + if err = bindingContext.Secret.Delete(); err != nil { logger.Error(err, "On deleting intermediary secret.") return RequeueError(err) } logger.Debug("Removing resource finalizers...") - sbr.SetFinalizers(removeStringSlice(sbr.GetFinalizers(), sbrFinalizer)) - if _, err = r.updateServiceBindingRequest(sbr); err != nil { + bindingContext.SBR.SetFinalizers(removeStringSlice(bindingContext.SBR.GetFinalizers(), sbrFinalizer)) + if _, err = r.updateServiceBindingRequest(bindingContext.SBR); err != nil { return NoRequeue(err) } @@ -235,26 +222,17 @@ func (r *Reconciler) unbind( // bind steps to bind backing service and applications together. It receive the elements collected // in the common parts of the reconciler, and execute the final binding steps. -func (r *Reconciler) bind( - logger *log.Log, - binder *Binder, - secret *Secret, - retrievedData map[string][]byte, - sbr *v1alpha1.ServiceBindingRequest, - sbrStatus *v1alpha1.ServiceBindingRequestStatus, - objectsToAnnotate []*unstructured.Unstructured, -) (reconcile.Result, error) { +func (r *Reconciler) bind(logger *log.Log, binder *Binder, bindingContext *BindingContext, sbrStatus *v1alpha1.ServiceBindingRequestStatus) (reconcile.Result, error) { logger = logger.WithName("bind") logger.Info("Saving data on intermediary secret...") - secretObj, err := secret.Commit(retrievedData) + secretObj, err := bindingContext.Secret.Commit(bindingContext.Data) + sbr := bindingContext.SBR if err != nil { logger.Error(err, "On saving secret data..") return r.onError(err, sbr, sbrStatus, nil) } - // appending intermediary secret in the list of objects to annotate - objectsToAnnotate = append(objectsToAnnotate, secretObj) // making sure secret name is part of status r.setSecretName(sbrStatus, secretObj.GetName()) @@ -270,7 +248,7 @@ func (r *Reconciler) bind( namespacedName := types.NamespacedName{Namespace: sbr.GetNamespace(), Name: sbr.GetName()} // annotating objects related to binding - if err = SetSBRAnnotations(r.dynClient, namespacedName, objectsToAnnotate); err != nil { + if err = SetSBRAnnotations(r.dynClient, namespacedName, bindingContext.Objects); err != nil { logger.Error(err, "On setting annotations in related objects.") return r.onError(err, sbr, sbrStatus, updatedObjects) } @@ -302,7 +280,6 @@ func (r *Reconciler) bind( // Deployment (and other kinds) will be updated in "spec" level. func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { ctx := context.TODO() - objectsToAnnotate := []*unstructured.Unstructured{} logger := reconcilerLog.WithValues( "Request.Namespace", request.Namespace, @@ -324,48 +301,104 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // splitting instance from it's status sbrStatus := &sbr.Status - // - // Planing changes - // + options := &BindingContextOptions{ + Client: r.dynClient, + DetectBindingResources: sbr.Spec.DetectBindingResources, + EnvVarPrefix: sbr.Spec.EnvVarPrefix, + SBR: sbr, + } - logger.Debug("Creating a plan based on OLM and CRD.") - planner := NewPlanner(ctx, r.dynClient, sbr) - plan, err := planner.Plan() + bindingContext, err := BuildBindingContext(options) if err != nil { - logger.Error(err, "On creating a plan to bind applications.") + logger.Error(err, "Creating binding context") return r.onError(err, sbr, sbrStatus, nil) } - // storing CR in objects to annotate - objectsToAnnotate = append(objectsToAnnotate, plan.CR) - // - // Retrieving data + // Binding and unbind intermediary secret // + binder := NewBinder(ctx, r.client, r.dynClient, sbr, bindingContext.VolumeKeys) - logger.Debug("Retrieving data to create intermediate secret.") - retriever := NewRetriever(r.dynClient, plan, sbr.Spec.EnvVarPrefix) - retrievedData, err := retriever.Retrieve() + if sbr.GetDeletionTimestamp() != nil { + logger.Info("Resource is marked for deletion...") + return r.unbind(logger, binder, bindingContext) + } + + logger.Info("Starting the bind of application(s) with backing service...") + return r.bind(logger, binder, bindingContext, sbrStatus) +} + +type BindingContext struct { + Data map[string][]byte + Objects []*unstructured.Unstructured + SBR *v1alpha1.ServiceBindingRequest + Secret *Secret + VolumeKeys []string +} + +type BindingContextOptions struct { + Client dynamic.Interface + CR *unstructured.Unstructured + DetectBindingResources bool + EnvVarPrefix string + Plan *Plan + SBR *v1alpha1.ServiceBindingRequest +} + +func BuildBindingContext(options *BindingContextOptions) (*BindingContext, error) { + ctx := context.Background() + + plan, err := BuildPlan(ctx, options.Client, options.SBR) if err != nil { - logger.Error(err, "On retrieving binding data.") - return r.onError(err, sbr, sbrStatus, nil) + return nil, err } - // storing objects used in Retriever - objectsToAnnotate = append(objectsToAnnotate, retriever.Objects...) + // storing CR in objects to annotate + retriever := NewRetriever(options.Client, options.Plan, options.EnvVarPrefix) + + if options.DetectBindingResources { + err := retriever.ReadBindableResourcesData( + &options.Plan.SBR, options.Plan.CR, []schema.GroupVersionResource{ + {Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "", Version: "v1", Resource: "services"}, + {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, + }, + ) + if err != nil { + return nil, err + } + } - // - // Binding and unbind intermediary secret - // + err = retriever.ReadCRDDescriptionData(options.Plan.CRDDescription) + if err != nil { + return nil, err + } - secret := NewSecret(r.dynClient, plan) - binder := NewBinder(ctx, r.client, r.dynClient, sbr, retriever.volumeKeys) + retrievedData, err := retriever.Retrieve() + if err != nil { + return nil, err + } - if sbr.GetDeletionTimestamp() != nil { - logger.Info("Resource is marked for deletion...") - return r.unbind(logger, binder, secret, sbr, objectsToAnnotate) + secret := NewSecret(options.Client, plan) + secretObj, err := secret.Get() + if err != nil { + return nil, err } - logger.Info("Starting the bind of application(s) with backing service...") - return r.bind(logger, binder, secret, retrievedData, sbr, sbrStatus, objectsToAnnotate) + return &BindingContext{ + SBR: options.SBR, + Objects: append([]*unstructured.Unstructured{}, plan.CR, secretObj), + Data: retrievedData, + VolumeKeys: retriever.volumeKeys, + Secret: secret, + }, nil +} + +func BuildPlan( + ctx context.Context, + dynClient dynamic.Interface, + sbr *v1alpha1.ServiceBindingRequest, +) (*Plan, error) { + planner := NewPlanner(ctx, dynClient, sbr) + return planner.Plan() } diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go index ae897b80ac..60bdb404c5 100644 --- a/pkg/controller/servicebindingrequest/retriever.go +++ b/pkg/controller/servicebindingrequest/retriever.go @@ -5,11 +5,13 @@ import ( "fmt" "strings" + olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" "github.com/redhat-developer/service-binding-operator/pkg/log" ) @@ -264,43 +266,53 @@ func (r *Retriever) store(key string, value []byte) { r.data[key] = value } -// Retrieve loop and read data pointed by the references in plan instance. Also runs through -// "bindable resources", gathering extra data. It can return error on retrieving and reading -// resources. +// Retrieve returns the data read from related resources (see ReadBindableResourcesData and +// ReadCRDDescriptionData). func (r *Retriever) Retrieve() (map[string][]byte, error) { - var err error - r.logger.Info("Looking for spec-descriptors in 'spec'...") - for _, specDescriptor := range r.plan.CRDDescription.SpecDescriptors { - if err = r.read("spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { - return nil, err - } - } + return r.data, nil +} - r.logger.Info("Looking for status-descriptors in 'status'...") - for _, statusDescriptor := range r.plan.CRDDescription.StatusDescriptors { - if err = r.read("status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { - return nil, err - } +// ReadBindableResourcesData reads all related resources of a given sbr +func (r *Retriever) ReadBindableResourcesData( + sbr *v1alpha1.ServiceBindingRequest, + cr *unstructured.Unstructured, + resources []schema.GroupVersionResource, +) error { + r.logger.Info("Detecting extra resources for binding...") + b := NewDetectBindableResources(sbr, cr, []schema.GroupVersionResource{ + {Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "", Version: "v1", Resource: "services"}, + {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, + }, r.client) + + vals, err := b.GetBindableVariables() + if err != nil { + return err + } + for k, v := range vals { + r.store(k, []byte(fmt.Sprintf("%v", v))) } - if r.plan.SBR.Spec.DetectBindingResources { - r.logger.Info("Detecting extra resources for binding...") - b := NewDetectBindableResources(&r.plan.SBR, r.plan.CR, []schema.GroupVersionResource{ - {Group: "", Version: "v1", Resource: "configmaps"}, - {Group: "", Version: "v1", Resource: "services"}, - {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, - }, r.client) + return nil +} - vals, err := b.GetBindableVariables() - if err != nil { - return nil, err +// ReadCRDDescriptionData reads data related to given crdDescription +func (r *Retriever) ReadCRDDescriptionData(crdDescription *olmv1alpha1.CRDDescription) error { + r.logger.Info("Looking for spec-descriptors in 'spec'...") + for _, specDescriptor := range crdDescription.SpecDescriptors { + if err := r.read("spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { + return err } - for k, v := range vals { - r.store(k, []byte(fmt.Sprintf("%v", v))) + } + + r.logger.Info("Looking for status-descriptors in 'status'...") + for _, statusDescriptor := range crdDescription.StatusDescriptors { + if err := r.read("status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { + return err } } - return r.data, nil + return nil } // NewRetriever instantiate a new retriever instance. diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go index 4e06058cae..bd16bf4652 100644 --- a/pkg/controller/servicebindingrequest/retriever_test.go +++ b/pkg/controller/servicebindingrequest/retriever_test.go @@ -32,7 +32,8 @@ func TestRetriever(t *testing.T) { retriever = NewRetriever(fakeDynClient, plan, "SERVICE_BINDING") require.NotNil(t, retriever) - t.Run("retrive", func(t *testing.T) { + t.Run("retrieve", func(t *testing.T) { + _ = retriever.ReadCRDDescriptionData(&crdDescription) objs, err := retriever.Retrieve() require.NoError(t, err) require.NotEmpty(t, retriever.data) @@ -222,6 +223,7 @@ func TestCustomEnvParser(t *testing.T) { require.NotNil(t, retriever) t.Run("Should detect custom env values", func(t *testing.T) { + _ = retriever.ReadCRDDescriptionData(&crdDescription) _, err = retriever.Retrieve() require.NoError(t, err) diff --git a/test/e2e/servicebindingrequest_test.go b/test/e2e/servicebindingrequest_test.go index 0fc45bd1d9..4bc023f1d7 100644 --- a/test/e2e/servicebindingrequest_test.go +++ b/test/e2e/servicebindingrequest_test.go @@ -24,6 +24,7 @@ import ( "github.com/redhat-developer/service-binding-operator/pkg/apis" "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" sbrcontroller "github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest" + "github.com/redhat-developer/service-binding-operator/pkg/converter" "github.com/redhat-developer/service-binding-operator/test/mocks" ) @@ -339,7 +340,9 @@ func CreateSBR( t.Logf("Creating ServiceBindingRequest mock object '%#v'...", namespacedName) sbr := mocks.ServiceBindingRequestMock( namespacedName.Namespace, namespacedName.Name, resourceRef, "", applicationGVR, matchLabels, false) - require.NoError(t, f.Client.Create(ctx, sbr, cleanupOpts)) + u, err := converter.ToUnstructured(sbr) + require.NoError(t, err) + require.NoError(t, f.Client.Create(ctx, u, cleanupOpts)) return sbr } From f5b5664740a73c51abf26ea383fc4faf0d0c81e8 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 19 Dec 2019 11:04:34 +0100 Subject: [PATCH 06/29] Add TypeMeta information in SBR mock --- test/mocks/mocks.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/mocks/mocks.go b/test/mocks/mocks.go index 119eabe59a..41034da255 100644 --- a/test/mocks/mocks.go +++ b/test/mocks/mocks.go @@ -380,6 +380,10 @@ func ServiceBindingRequestMock( bindUnannotated bool, ) *v1alpha1.ServiceBindingRequest { return &v1alpha1.ServiceBindingRequest{ + TypeMeta: metav1.TypeMeta{ + Kind: "ServiceBindingRequest", + APIVersion: v1alpha1.SchemeGroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Namespace: ns, Name: name, @@ -423,7 +427,7 @@ func UnstructuredServiceBindingRequestMock( ) (*unstructured.Unstructured, error) { sbr := ServiceBindingRequestMock( ns, name, backingServiceResourceRef, applicationResourceRef, applicationGVR, matchLabels, false) - return converter.ToUnstructuredAsGVK(&sbr, v1alpha1.SchemeGroupVersion.WithKind(OperatorKind)) + return converter.ToUnstructured(sbr) } // DeploymentConfigListMock returns a list of DeploymentMock. From 495ddfca3a61e70b34995aca62fc970e4a5ccc18 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 19 Dec 2019 11:04:54 +0100 Subject: [PATCH 07/29] Mark yaml encoding to inline GroupVersionKind --- pkg/apis/apps/v1alpha1/servicebindingrequest_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go index 6c800f4fdd..d4451bddc4 100644 --- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go +++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go @@ -56,7 +56,7 @@ type ServiceBindingRequestStatus struct { // BackingServiceSelector defines the selector based on resource name, version, and resource kind // +k8s:openapi-gen=true type BackingServiceSelector struct { - schema.GroupVersionKind `json:",inline"` + schema.GroupVersionKind `json:",inline" yaml:",inline"` ResourceRef string `json:"resourceRef"` } From 58028002e38aba771c293977d23b5775c5f41a90 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 16:38:52 +0100 Subject: [PATCH 08/29] Split Reconciiler into Reconciler and BindingManager --- .../servicebindingrequest/binding.go | 329 ++++++++++++++++++ .../servicebindingrequest/common.go | 11 - .../servicebindingrequest/reconciler.go | 290 ++------------- .../servicebindingrequest/reconciler_test.go | 15 +- .../servicebindingrequest/retriever.go | 6 +- 5 files changed, 358 insertions(+), 293 deletions(-) create mode 100644 pkg/controller/servicebindingrequest/binding.go diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go new file mode 100644 index 0000000000..bfe1aa819a --- /dev/null +++ b/pkg/controller/servicebindingrequest/binding.go @@ -0,0 +1,329 @@ +package servicebindingrequest + +import ( + "context" + "fmt" + + "gotest.tools/assert/cmp" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" + "github.com/redhat-developer/service-binding-operator/pkg/converter" + "github.com/redhat-developer/service-binding-operator/pkg/log" +) + +const ( + // bindingInProgress binding is in progress + bindingInProgress = "InProgress" + // bindingFail binding has failed + bindingFail = "Fail" + // time in seconds to wait before requeuing requests + requeueAfter int64 = 45 + // Finalizer annotation used in finalizer steps + Finalizer = "finalizer.servicebindingrequest.openshift.io" +) + +// BindingManager manages binding for a Service Binding Request and associated objects. +type BindingManager struct { + // Binder is responsible for interacting with the cluster and apply binding related changes. + Binder *Binder + // Data is the collection of all data read by the manager. + Data map[string][]byte + // DynClient is the Kubernetes dynamic client used to interact with the cluster. + DynClient dynamic.Interface + // Logger provides logging facilities for internal components. + Logger *log.Log + // Objects is a list of additional unstructured objects related to the Service Binding Request. + Objects []*unstructured.Unstructured + // SBR is the ServiceBindingRequest associated with binding.. + SBR *v1alpha1.ServiceBindingRequest + // Secret is the Secret associated with the Service Binding Request. + Secret *Secret +} + +// removeStringSlice given a string slice and a string, returns a new slice without given string. +func removeStringSlice(slice []string, str string) []string { + var cleanSlice []string + for _, s := range slice { + if str != s { + cleanSlice = append(cleanSlice, s) + } + } + return cleanSlice +} + +// Unbind removes the relationship between a Service Binding Request and its related objects. +func (b *BindingManager) Unbind() (reconcile.Result, error) { + logger := b.Logger.WithName("Unbind") + + logger.Info("Cleaning related objects from operator's annotations...") + if err := RemoveSBRAnnotations(b.DynClient, b.Objects); err != nil { + logger.Error(err, "On removing annotations from related objects.") + return RequeueError(err) + } + + if err := b.Binder.Unbind(); err != nil { + logger.Error(err, "On unbinding related objects") + return RequeueError(err) + } + + logger.Info("Deleting intermediary secret") + if err := b.Secret.Delete(); err != nil { + logger.Error(err, "On deleting intermediary secret.") + return RequeueError(err) + } + + logger.Debug("Removing resource finalizers...") + b.SBR.SetFinalizers(removeStringSlice(b.SBR.GetFinalizers(), Finalizer)) + if _, err := b.updateServiceBindingRequest(b.SBR); err != nil { + return NoRequeue(err) + } + + return Done() +} + +// updateServiceBindingRequest execute update API call on a SBR request. It can return errors from +// this action. +func (b *BindingManager) updateServiceBindingRequest( + sbr *v1alpha1.ServiceBindingRequest, +) (*v1alpha1.ServiceBindingRequest, error) { + u, err := converter.ToUnstructured(sbr) + if err != nil { + return nil, err + } + + u, err = b.DynClient. + Resource(GroupVersion). + Namespace(sbr.GetNamespace()). + Update(u, v1.UpdateOptions{}) + + if err != nil { + return nil, err + } + + err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) + if err != nil { + return nil, err + } + return sbr, nil +} + +// onError comprise the update of ServiceBindingRequest status to set error flag, and inspect +// informed error to apply a different behavior for not-founds. +func (b *BindingManager) onError( + err error, + sbr *v1alpha1.ServiceBindingRequest, + sbrStatus *v1alpha1.ServiceBindingRequestStatus, + objs []*unstructured.Unstructured, +) (reconcile.Result, error) { + sbrStatus.BindingStatus = bindingFail + + if objs != nil { + sbrStatus.BindingStatus = BindingSuccess + b.setApplicationObjects(sbrStatus, objs) + } + + _, errStatus := b.updateStatusServiceBindingRequest(sbr, sbrStatus) + if errStatus != nil { + return RequeueError(errStatus) + } + + return RequeueOnNotFound(err, requeueAfter) +} + +// Bind configures binding between the Service Binding Request and its related objects. +func (b *BindingManager) Bind() (reconcile.Result, error) { + sbrStatus := b.SBR.Status.DeepCopy() + + b.Logger.Info("Saving data on intermediary secret...") + secretObj, err := b.Secret.Commit(b.Data) + if err != nil { + b.Logger.Error(err, "On saving secret data..") + return b.onError(err, b.SBR, sbrStatus, nil) + } + + // update status information + sbrStatus.BindingStatus = bindingInProgress + sbrStatus.Secret = secretObj.GetName() + + updatedObjects, err := b.Binder.Bind() + if err != nil { + b.Logger.Error(err, "On binding application.") + return b.onError(err, b.SBR, sbrStatus, updatedObjects) + } + + // saving on status the list of objects that have been touched + sbrStatus.BindingStatus = BindingSuccess + b.setApplicationObjects(sbrStatus, updatedObjects) + + // annotating objects related to binding + namespacedName := types.NamespacedName{Namespace: b.SBR.GetNamespace(), Name: b.SBR.GetName()} + if err = SetSBRAnnotations(b.DynClient, namespacedName, b.Objects); err != nil { + b.Logger.Error(err, "On setting annotations in related objects.") + return b.onError(err, b.SBR, sbrStatus, updatedObjects) + } + + // updating status of request instance + sbr, err := b.updateStatusServiceBindingRequest(b.SBR, sbrStatus) + if err != nil { + return RequeueOnConflict(err) + } + + // appending finalizer, should be later removed upon resource deletion + sbr.SetFinalizers(append(sbr.GetFinalizers(), Finalizer)) + if _, err = b.updateServiceBindingRequest(sbr); err != nil { + return NoRequeue(err) + } + + b.Logger.Info("All done!") + return Done() +} + +// updateStatusServiceBindingRequest updates the Service Binding Request's status field. +func (b *BindingManager) updateStatusServiceBindingRequest( + sbr *v1alpha1.ServiceBindingRequest, + sbrStatus *v1alpha1.ServiceBindingRequestStatus, +) ( + *v1alpha1.ServiceBindingRequest, + error, +) { + // do not update if both statuses are equal + if result := cmp.DeepEqual(sbr.Status, sbrStatus)(); result.Success() { + return sbr, nil + } + + // coping status over informed object + sbr.Status = *sbrStatus + + // converting object into unstructured + u, err := converter.ToUnstructured(sbr) + if err != nil { + return nil, err + } + + gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) + resourceClient := b.DynClient.Resource(gr).Namespace(sbr.GetNamespace()) + u, err = resourceClient.UpdateStatus(u, v1.UpdateOptions{}) + if err != nil { + return nil, err + } + + err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) + if err != nil { + return nil, err + } + return sbr, nil +} + +// setApplicationObjects replaces the Status's equivalent field. +func (b *BindingManager) setApplicationObjects( + sbrStatus *v1alpha1.ServiceBindingRequestStatus, + objs []*unstructured.Unstructured, +) { + names := []string{} + for _, obj := range objs { + names = append(names, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName())) + } + sbrStatus.ApplicationObjects = names +} + +// GroupVersion represents the service binding request resource's group version. +var GroupVersion = v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) + +// BindingManagerOptions is BuildBindingManager arguments. +type BindingManagerOptions struct { + Logger *log.Log + DynClient dynamic.Interface + CR *unstructured.Unstructured + DetectBindingResources bool + EnvVarPrefix string + SBR *v1alpha1.ServiceBindingRequest + Client client.Client +} + +// buildPlan creates a new plan. +func buildPlan( + ctx context.Context, + dynClient dynamic.Interface, + sbr *v1alpha1.ServiceBindingRequest, +) (*Plan, error) { + planner := NewPlanner(ctx, dynClient, sbr) + return planner.Plan() +} + +// BuildBindingManager creates a new binding manager according to options. +func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error) { + ctx := context.Background() + + // objs groups all extra objects related to the informed SBR + objs := make([]*unstructured.Unstructured, 0, 0) + + // plan is a source of information regarding the binding process + plan, err := buildPlan(ctx, options.DynClient, options.SBR) + if err != nil { + return nil, err + } + + if plan.CR != nil { + // append only if contains a value + objs = append(objs, plan.CR) + } + + // retriever is responsible for gathering data related to the given plan. + retriever := NewRetriever(options.DynClient, plan, options.EnvVarPrefix) + + // read bindable data from the specified resources + if options.DetectBindingResources { + err := retriever.ReadBindableResourcesData( + &plan.SBR, plan.CR, []schema.GroupVersionResource{ + {Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "", Version: "v1", Resource: "services"}, + {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, + }, + ) + if err != nil { + return nil, err + } + } + + // read bindable data from the CRDDescription found by the planner + err = retriever.ReadCRDDescriptionData(plan.CRDDescription) + if err != nil { + return nil, err + } + + // gather retriever's read data + // TODO: do not return error + retrievedData, err := retriever.Retrieve() + if err != nil { + return nil, err + } + + // gather related secret, again only appending it if there's a value. + secret := NewSecret(options.DynClient, plan) + secretObj, err := secret.Get() + if !errors.IsNotFound(err) { + return nil, err + } + if secretObj != nil { + objs = append(objs, secretObj) + } + + return &BindingManager{ + Logger: options.Logger, + Binder: NewBinder(ctx, options.Client, options.DynClient, options.SBR, retriever.VolumeKeys), + DynClient: options.DynClient, + SBR: options.SBR, + Objects: objs, + Data: retrievedData, + Secret: secret, + }, nil +} diff --git a/pkg/controller/servicebindingrequest/common.go b/pkg/controller/servicebindingrequest/common.go index 5984217936..43f0f54c00 100644 --- a/pkg/controller/servicebindingrequest/common.go +++ b/pkg/controller/servicebindingrequest/common.go @@ -78,14 +78,3 @@ func containsStringSlice(slice []string, str string) bool { } return false } - -// removeStringSlice given a string slice and a string, returns a new slice without given string. -func removeStringSlice(slice []string, str string) []string { - var cleanSlice []string - for _, s := range slice { - if str != s { - cleanSlice = append(cleanSlice, s) - } - } - return cleanSlice -} diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index 99fc96c397..61253ef183 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -1,21 +1,16 @@ package servicebindingrequest import ( - "context" "fmt" - "gotest.tools/assert/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" - "github.com/redhat-developer/service-binding-operator/pkg/converter" "github.com/redhat-developer/service-binding-operator/pkg/log" ) @@ -27,46 +22,12 @@ type Reconciler struct { } const ( - // bindingInProgress binding is in progress - bindingInProgress = "InProgress" - // BindingSuccess binding has succeeded BindingSuccess = "Success" - // bindingFail binding has failed - bindingFail = "Fail" - // time in seconds to wait before requeuing requests - requeueAfter int64 = 45 - // sbrFinalizer annotation used in finalizer steps - sbrFinalizer = "finalizer.servicebindingrequest.openshift.io" ) // reconcilerLog local logger instance var reconcilerLog = log.NewLog("reconciler") -// setSecretName update the CR status field to "in progress", and setting secret name. -func (r *Reconciler) setSecretName(sbrStatus *v1alpha1.ServiceBindingRequestStatus, name string) { - sbrStatus.BindingStatus = bindingInProgress - sbrStatus.Secret = name -} - -// setStatus update the CR status field. -func (r *Reconciler) setStatus(sbrStatus *v1alpha1.ServiceBindingRequestStatus, status string) { - sbrStatus.BindingStatus = status -} - -// setApplicationObjects set the ApplicationObject status field, and also set the overall status as -// success, since it was able to bind applications. -func (r *Reconciler) setApplicationObjects( - sbrStatus *v1alpha1.ServiceBindingRequestStatus, - objs []*unstructured.Unstructured, -) { - names := []string{} - for _, obj := range objs { - names = append(names, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName())) - } - sbrStatus.BindingStatus = BindingSuccess - sbrStatus.ApplicationObjects = names -} - // validateServiceBindingRequest check for unsupported settings in SBR. func (r *Reconciler) validateServiceBindingRequest(sbr *v1alpha1.ServiceBindingRequest) error { // check if application ResourceRef and MatchLabels, one of them is required. @@ -100,120 +61,22 @@ func (r *Reconciler) getServiceBindingRequest( return sbr, nil } -// updateStatusServiceBindingRequest update the status field of a ServiceBindingRequest. -func (r *Reconciler) updateStatusServiceBindingRequest( - sbr *v1alpha1.ServiceBindingRequest, - sbrStatus *v1alpha1.ServiceBindingRequestStatus, -) (*v1alpha1.ServiceBindingRequest, error) { - // do not update if both statuses are equal - if result := cmp.DeepEqual(sbr.Status, sbrStatus)(); result.Success() { - return sbr, nil - } - - // coping status over informed object - sbr.Status = *sbrStatus - - // converting object into unstructured - u, err := converter.ToUnstructured(sbr) - if err != nil { - return nil, err - } - - gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) - resourceClient := r.dynClient.Resource(gr).Namespace(sbr.GetNamespace()) - u, err = resourceClient.UpdateStatus(u, metav1.UpdateOptions{}) - if err != nil { - return nil, err - } - - err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) - if err != nil { - return nil, err - } - return sbr, nil -} - -// updateServiceBindingRequest execute update API call on a SBR request. It can return errors from -// this action. -func (r *Reconciler) updateServiceBindingRequest( - sbr *v1alpha1.ServiceBindingRequest, -) (*v1alpha1.ServiceBindingRequest, error) { - u, err := converter.ToUnstructured(sbr) - if err != nil { - return nil, err - } - gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) - resourceClient := r.dynClient.Resource(gr).Namespace(sbr.GetNamespace()) - u, err = resourceClient.Update(u, metav1.UpdateOptions{}) - if err != nil { - return nil, err - } - - err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) - if err != nil { - return nil, err - } - return sbr, nil -} - -// onError comprise the update of ServiceBindingRequest status to set error flag, and inspect -// informed error to apply a different behavior for not-founds. -func (r *Reconciler) onError( - err error, - sbr *v1alpha1.ServiceBindingRequest, - sbrStatus *v1alpha1.ServiceBindingRequestStatus, - objs []*unstructured.Unstructured, -) (reconcile.Result, error) { - // settting overall status to failed - r.setStatus(sbrStatus, bindingFail) - // - if objs != nil { - r.setApplicationObjects(sbrStatus, objs) - } - _, errStatus := r.updateStatusServiceBindingRequest(sbr, sbrStatus) - if errStatus != nil { - return RequeueError(errStatus) - } - return RequeueOnNotFound(err, requeueAfter) -} - // unbind removes the relationship between the given sbr and the manifests the operator has // previously modified. This process also deletes any manifests created to support the binding // functionality, such as ConfigMaps and Secrets. -func (r *Reconciler) unbind(logger *log.Log, binder *Binder, bindingContext *BindingContext) (reconcile.Result, error) { - var err error - +func (r *Reconciler) unbind(logger *log.Log, bindingContext *BindingManager) (reconcile.Result, error) { logger = logger.WithName("unbind") // when finalizer is not found anymore, it can be safely removed - if !containsStringSlice(bindingContext.SBR.GetFinalizers(), sbrFinalizer) { + if !containsStringSlice(bindingContext.SBR.GetFinalizers(), Finalizer) { logger.Info("Resource can be safely deleted!") return Done() } - logger.Info("Cleaning related objects from operator's annotations...") - if err = RemoveSBRAnnotations(r.dynClient, bindingContext.Objects); err != nil { - logger.Error(err, "On removing annotations from related objects.") - return RequeueError(err) - } - logger.Info("Executing unbinding steps...") - err = binder.Unbind() - if err != nil { + if res, err := bindingContext.Unbind(); err != nil { logger.Error(err, "On unbinding application.") - return RequeueError(err) - } - - logger.Info("Deleting intermediary secret...") - if err = bindingContext.Secret.Delete(); err != nil { - logger.Error(err, "On deleting intermediary secret.") - return RequeueError(err) - } - - logger.Debug("Removing resource finalizers...") - bindingContext.SBR.SetFinalizers(removeStringSlice(bindingContext.SBR.GetFinalizers(), sbrFinalizer)) - if _, err = r.updateServiceBindingRequest(bindingContext.SBR); err != nil { - return NoRequeue(err) + return res, err } logger.Debug("Deletion done!") @@ -222,50 +85,18 @@ func (r *Reconciler) unbind(logger *log.Log, binder *Binder, bindingContext *Bin // bind steps to bind backing service and applications together. It receive the elements collected // in the common parts of the reconciler, and execute the final binding steps. -func (r *Reconciler) bind(logger *log.Log, binder *Binder, bindingContext *BindingContext, sbrStatus *v1alpha1.ServiceBindingRequestStatus) (reconcile.Result, error) { +func (r *Reconciler) bind( + logger *log.Log, + bindingContext *BindingManager, + sbrStatus *v1alpha1.ServiceBindingRequestStatus, +) ( + reconcile.Result, + error, +) { logger = logger.WithName("bind") - logger.Info("Saving data on intermediary secret...") - secretObj, err := bindingContext.Secret.Commit(bindingContext.Data) - sbr := bindingContext.SBR - if err != nil { - logger.Error(err, "On saving secret data..") - return r.onError(err, sbr, sbrStatus, nil) - } - - // making sure secret name is part of status - r.setSecretName(sbrStatus, secretObj.GetName()) - logger.Info("Binding applications with intermediary secret...") - updatedObjects, err := binder.Bind() - if err != nil { - logger.Error(err, "On binding application.") - return r.onError(err, sbr, sbrStatus, updatedObjects) - } - - // saving on status the list of objects that have been touched - r.setApplicationObjects(sbrStatus, updatedObjects) - namespacedName := types.NamespacedName{Namespace: sbr.GetNamespace(), Name: sbr.GetName()} - - // annotating objects related to binding - if err = SetSBRAnnotations(r.dynClient, namespacedName, bindingContext.Objects); err != nil { - logger.Error(err, "On setting annotations in related objects.") - return r.onError(err, sbr, sbrStatus, updatedObjects) - } - - // updating status of request instance - if sbr, err = r.updateStatusServiceBindingRequest(sbr, sbrStatus); err != nil { - return RequeueOnConflict(err) - } - - // appending finalizer, should be later removed upon resource deletion - sbr.SetFinalizers(append(sbr.GetFinalizers(), sbrFinalizer)) - if _, err = r.updateServiceBindingRequest(sbr); err != nil { - return NoRequeue(err) - } - - logger.Info("All done!") - return Done() + return bindingContext.Bind() } // Reconcile a ServiceBindingRequest by the following steps: @@ -279,8 +110,6 @@ func (r *Reconciler) bind(logger *log.Log, binder *Binder, bindingContext *Bindi // 4. Search applications that are interested to bind with given service, by inspecting labels. The // Deployment (and other kinds) will be updated in "spec" level. func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { - ctx := context.TODO() - logger := reconcilerLog.WithValues( "Request.Namespace", request.Namespace, "Request.Name", request.Name, @@ -301,104 +130,25 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // splitting instance from it's status sbrStatus := &sbr.Status - options := &BindingContextOptions{ - Client: r.dynClient, + options := &BindingManagerOptions{ + DynClient: r.dynClient, DetectBindingResources: sbr.Spec.DetectBindingResources, EnvVarPrefix: sbr.Spec.EnvVarPrefix, SBR: sbr, + Logger: logger, } - bindingContext, err := BuildBindingContext(options) + bindingContext, err := BuildBindingManager(options) if err != nil { logger.Error(err, "Creating binding context") - return r.onError(err, sbr, sbrStatus, nil) + return RequeueError(err) } - // - // Binding and unbind intermediary secret - // - binder := NewBinder(ctx, r.client, r.dynClient, sbr, bindingContext.VolumeKeys) - if sbr.GetDeletionTimestamp() != nil { logger.Info("Resource is marked for deletion...") - return r.unbind(logger, binder, bindingContext) + return r.unbind(logger, bindingContext) } logger.Info("Starting the bind of application(s) with backing service...") - return r.bind(logger, binder, bindingContext, sbrStatus) -} - -type BindingContext struct { - Data map[string][]byte - Objects []*unstructured.Unstructured - SBR *v1alpha1.ServiceBindingRequest - Secret *Secret - VolumeKeys []string -} - -type BindingContextOptions struct { - Client dynamic.Interface - CR *unstructured.Unstructured - DetectBindingResources bool - EnvVarPrefix string - Plan *Plan - SBR *v1alpha1.ServiceBindingRequest -} - -func BuildBindingContext(options *BindingContextOptions) (*BindingContext, error) { - ctx := context.Background() - - plan, err := BuildPlan(ctx, options.Client, options.SBR) - if err != nil { - return nil, err - } - - // storing CR in objects to annotate - retriever := NewRetriever(options.Client, options.Plan, options.EnvVarPrefix) - - if options.DetectBindingResources { - err := retriever.ReadBindableResourcesData( - &options.Plan.SBR, options.Plan.CR, []schema.GroupVersionResource{ - {Group: "", Version: "v1", Resource: "configmaps"}, - {Group: "", Version: "v1", Resource: "services"}, - {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, - }, - ) - if err != nil { - return nil, err - } - } - - err = retriever.ReadCRDDescriptionData(options.Plan.CRDDescription) - if err != nil { - return nil, err - } - - retrievedData, err := retriever.Retrieve() - if err != nil { - return nil, err - } - - secret := NewSecret(options.Client, plan) - secretObj, err := secret.Get() - if err != nil { - return nil, err - } - - return &BindingContext{ - SBR: options.SBR, - Objects: append([]*unstructured.Unstructured{}, plan.CR, secretObj), - Data: retrievedData, - VolumeKeys: retriever.volumeKeys, - Secret: secret, - }, nil -} - -func BuildPlan( - ctx context.Context, - dynClient dynamic.Interface, - sbr *v1alpha1.ServiceBindingRequest, -) (*Plan, error) { - planner := NewPlanner(ctx, dynClient, sbr) - return planner.Plan() + return r.bind(logger, bindingContext, sbrStatus) } diff --git a/pkg/controller/servicebindingrequest/reconciler_test.go b/pkg/controller/servicebindingrequest/reconciler_test.go index dc52b13872..e6c81163c0 100644 --- a/pkg/controller/servicebindingrequest/reconciler_test.go +++ b/pkg/controller/servicebindingrequest/reconciler_test.go @@ -46,7 +46,9 @@ func TestReconcilerReconcileError(t *testing.T) { "environment": "reconciler", } f := mocks.NewFake(t, reconcilerNs) + f.AddMockedUnstructuredDatabaseCRD() f.AddMockedUnstructuredServiceBindingRequest(reconcilerName, backingServiceResourceRef, "", deploymentsGVR, matchLabels) + f.AddMockedUnstructuredPostgresDatabaseCR("test-using-secret") fakeClient := f.FakeClient() fakeDynClient := f.FakeDynClient() @@ -54,15 +56,10 @@ func TestReconcilerReconcileError(t *testing.T) { res, err := reconciler.Reconcile(reconcileRequest()) - // FIXME: decide this test's fate - // I'm not very sure what this test was about, but in the case the SBR definition contains - // references to objects that do not exist, the reconciliation process is supposed to be - // successful. Commented below was the original test. - // - // require.Error(t, err) - // require.True(t, res.Requeue) - - require.NoError(t, err) + // currently this test passes because annotations present in the Databases CRD being currently + // used doesn't have a 'status' field in its definition; once it does and this code is updated ( + // since the Postgres CRD is being imported to be used in tests) this test will fail. + require.Error(t, err) require.True(t, res.Requeue) } diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go index 60bdb404c5..744d7b3dc9 100644 --- a/pkg/controller/servicebindingrequest/retriever.go +++ b/pkg/controller/servicebindingrequest/retriever.go @@ -22,7 +22,7 @@ type Retriever struct { Objects []*unstructured.Unstructured // list of objects employed client dynamic.Interface // Kubernetes API client plan *Plan // plan instance - volumeKeys []string // list of keys found + VolumeKeys []string // list of keys found bindingPrefix string // prefix for variable names cache map[string]interface{} // store visited paths } @@ -114,7 +114,7 @@ func (r *Retriever) read(place, path string, xDescriptors []string) error { } else if strings.HasPrefix(xDescriptor, volumeMountSecretPrefix) { secrets[pathValue] = append(secrets[pathValue], r.extractSecretItemName(xDescriptor)) r.markVisitedPaths(r.extractSecretItemName(xDescriptor), pathValue, place) - r.volumeKeys = append(r.volumeKeys, pathValue) + r.VolumeKeys = append(r.VolumeKeys, pathValue) } else if strings.HasPrefix(xDescriptor, attributePrefix) { r.store(path, []byte(pathValue)) } else { @@ -323,7 +323,7 @@ func NewRetriever(client dynamic.Interface, plan *Plan, bindingPrefix string) *R Objects: []*unstructured.Unstructured{}, client: client, plan: plan, - volumeKeys: []string{}, + VolumeKeys: []string{}, bindingPrefix: bindingPrefix, cache: make(map[string]interface{}), } From c106db9145e59b6c621b8fa6f7844e67650558d8 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 17:50:05 +0100 Subject: [PATCH 09/29] Add found bool return value It is expected the Secret to not exist if the binding hasn't been performed yet, so the API was changed to support this use case. Additionally, found is introduced to avoid importing "errors" and Kubernetes own errors package. --- pkg/controller/servicebindingrequest/binding.go | 9 +++------ pkg/controller/servicebindingrequest/secret.go | 8 ++++++-- pkg/controller/servicebindingrequest/secret_test.go | 3 ++- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index bfe1aa819a..1290f15940 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -2,10 +2,10 @@ package servicebindingrequest import ( "context" + "errors" "fmt" "gotest.tools/assert/cmp" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -309,11 +309,8 @@ func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error // gather related secret, again only appending it if there's a value. secret := NewSecret(options.DynClient, plan) - secretObj, err := secret.Get() - if !errors.IsNotFound(err) { - return nil, err - } - if secretObj != nil { + secretObj, found, err := secret.Get() + if found { objs = append(objs, secretObj) } diff --git a/pkg/controller/servicebindingrequest/secret.go b/pkg/controller/servicebindingrequest/secret.go index 280330f87a..3b08af83c6 100644 --- a/pkg/controller/servicebindingrequest/secret.go +++ b/pkg/controller/servicebindingrequest/secret.go @@ -94,9 +94,13 @@ func (s *Secret) Commit(data map[string][]byte) (*unstructured.Unstructured, err // Get an unstructured object from the secret handled by this component. It can return errors in case // the API server does. -func (s *Secret) Get() (*unstructured.Unstructured, error) { +func (s *Secret) Get() (*unstructured.Unstructured, bool, error) { resourceClient := s.buildResourceClient() - return resourceClient.Get(s.plan.Name, metav1.GetOptions{}) + u, err := resourceClient.Get(s.plan.Name, metav1.GetOptions{}) + if err != nil && !errors.IsNotFound(err) { + return nil, false, err + } + return u, u != nil, nil } // Delete the secret represented by this component. It can return error when the API server does. diff --git a/pkg/controller/servicebindingrequest/secret_test.go b/pkg/controller/servicebindingrequest/secret_test.go index f3f8988032..207e210505 100644 --- a/pkg/controller/servicebindingrequest/secret_test.go +++ b/pkg/controller/servicebindingrequest/secret_test.go @@ -52,8 +52,9 @@ func TestSecretNew(t *testing.T) { }) t.Run("Get", func(t *testing.T) { - u, err := s.Get() + u, found, err := s.Get() assert.NoError(t, err) + assert.True(t, found) assertSecretNamespacedName(t, u, ns, name) }) } From 54edba9cee0be5265bfb21f0ba21112bd17031f5 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 17:50:38 +0100 Subject: [PATCH 10/29] Client is required --- pkg/controller/servicebindingrequest/reconciler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index 61253ef183..ce32d5811b 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -131,6 +131,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err sbrStatus := &sbr.Status options := &BindingManagerOptions{ + Client: r.client, DynClient: r.dynClient, DetectBindingResources: sbr.Spec.DetectBindingResources, EnvVarPrefix: sbr.Spec.EnvVarPrefix, From 5384087bd8c3c743e7148e122d1825ecce3b6ee3 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 17:51:10 +0100 Subject: [PATCH 11/29] Move types and constant like vars to the top --- .../servicebindingrequest/binding.go | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index 1290f15940..75c57933ed 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -31,6 +31,24 @@ const ( Finalizer = "finalizer.servicebindingrequest.openshift.io" ) +// GroupVersion represents the service binding request resource's group version. +var GroupVersion = v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) + +// BindingManagerOptions is BuildBindingManager arguments. +type BindingManagerOptions struct { + Logger *log.Log + DynClient dynamic.Interface + DetectBindingResources bool + EnvVarPrefix string + SBR *v1alpha1.ServiceBindingRequest + Client client.Client +} + +// Valid returns whether the options are valid. +func (o *BindingManagerOptions) Valid() bool { + return o.SBR != nil && o.DynClient != nil && o.Client != nil +} + // BindingManager manages binding for a Service Binding Request and associated objects. type BindingManager struct { // Binder is responsible for interacting with the cluster and apply binding related changes. @@ -235,20 +253,6 @@ func (b *BindingManager) setApplicationObjects( sbrStatus.ApplicationObjects = names } -// GroupVersion represents the service binding request resource's group version. -var GroupVersion = v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) - -// BindingManagerOptions is BuildBindingManager arguments. -type BindingManagerOptions struct { - Logger *log.Log - DynClient dynamic.Interface - CR *unstructured.Unstructured - DetectBindingResources bool - EnvVarPrefix string - SBR *v1alpha1.ServiceBindingRequest - Client client.Client -} - // buildPlan creates a new plan. func buildPlan( ctx context.Context, From cd49930c70b274548d669a7e9cbace90d7ab240d Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 17:51:33 +0100 Subject: [PATCH 12/29] Validate options in BuildBindingManager --- pkg/controller/servicebindingrequest/binding.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index 75c57933ed..b0a41dc3e4 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -263,14 +263,20 @@ func buildPlan( return planner.Plan() } +// InvalidOptionsErr is returned when BindingManagerOptions are not valid. +var InvalidOptionsErr = errors.New("invalid options") + // BuildBindingManager creates a new binding manager according to options. func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error) { - ctx := context.Background() + if !options.Valid() { + return nil, InvalidOptionsErr + } // objs groups all extra objects related to the informed SBR objs := make([]*unstructured.Unstructured, 0, 0) // plan is a source of information regarding the binding process + ctx := context.Background() plan, err := buildPlan(ctx, options.DynClient, options.SBR) if err != nil { return nil, err From e838a17b141a8138cf30b218768c9c387217e788 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Mon, 30 Dec 2019 17:51:56 +0100 Subject: [PATCH 13/29] Rename local variable --- .../servicebindingrequest/reconciler.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index ce32d5811b..cc8a6d1cb3 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -64,17 +64,17 @@ func (r *Reconciler) getServiceBindingRequest( // unbind removes the relationship between the given sbr and the manifests the operator has // previously modified. This process also deletes any manifests created to support the binding // functionality, such as ConfigMaps and Secrets. -func (r *Reconciler) unbind(logger *log.Log, bindingContext *BindingManager) (reconcile.Result, error) { +func (r *Reconciler) unbind(logger *log.Log, bm *BindingManager) (reconcile.Result, error) { logger = logger.WithName("unbind") // when finalizer is not found anymore, it can be safely removed - if !containsStringSlice(bindingContext.SBR.GetFinalizers(), Finalizer) { + if !containsStringSlice(bm.SBR.GetFinalizers(), Finalizer) { logger.Info("Resource can be safely deleted!") return Done() } logger.Info("Executing unbinding steps...") - if res, err := bindingContext.Unbind(); err != nil { + if res, err := bm.Unbind(); err != nil { logger.Error(err, "On unbinding application.") return res, err } @@ -87,7 +87,7 @@ func (r *Reconciler) unbind(logger *log.Log, bindingContext *BindingManager) (re // in the common parts of the reconciler, and execute the final binding steps. func (r *Reconciler) bind( logger *log.Log, - bindingContext *BindingManager, + bm *BindingManager, sbrStatus *v1alpha1.ServiceBindingRequestStatus, ) ( reconcile.Result, @@ -96,7 +96,7 @@ func (r *Reconciler) bind( logger = logger.WithName("bind") logger.Info("Binding applications with intermediary secret...") - return bindingContext.Bind() + return bm.Bind() } // Reconcile a ServiceBindingRequest by the following steps: @@ -139,7 +139,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err Logger: logger, } - bindingContext, err := BuildBindingManager(options) + bm, err := BuildBindingManager(options) if err != nil { logger.Error(err, "Creating binding context") return RequeueError(err) @@ -147,9 +147,9 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err if sbr.GetDeletionTimestamp() != nil { logger.Info("Resource is marked for deletion...") - return r.unbind(logger, bindingContext) + return r.unbind(logger, bm) } logger.Info("Starting the bind of application(s) with backing service...") - return r.bind(logger, bindingContext, sbrStatus) + return r.bind(logger, bm, sbrStatus) } From c04eaa974f662225cf2b9ee165019c663e486716 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Tue, 31 Dec 2019 18:30:11 +0100 Subject: [PATCH 14/29] Reword field documentation --- pkg/apis/apps/v1alpha1/servicebindingrequest_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go index d4451bddc4..64530a9e29 100644 --- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go +++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go @@ -29,7 +29,7 @@ type ServiceBindingRequestSpec struct { // BackingServiceSelector is used to identify the backing service operator. BackingServiceSelector BackingServiceSelector `json:"backingServiceSelector"` - // BackingServiceSelectors is an slice of BackingServiceSelector + // BackingServiceSelectors is used to identify multiple backing services. BackingServiceSelectors []BackingServiceSelector `json:"backingServiceSelectors"` // ApplicationSelector is used to identify the application connecting to the From 8ae4ff91ea9a98292efeeeecdd2d1570ddc72732 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Tue, 31 Dec 2019 18:31:19 +0100 Subject: [PATCH 15/29] Organize imports --- pkg/controller/servicebindingrequest/bind_custom_resources.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/controller/servicebindingrequest/bind_custom_resources.go b/pkg/controller/servicebindingrequest/bind_custom_resources.go index 19469e9983..432ea80868 100644 --- a/pkg/controller/servicebindingrequest/bind_custom_resources.go +++ b/pkg/controller/servicebindingrequest/bind_custom_resources.go @@ -1,11 +1,12 @@ package servicebindingrequest import ( - "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" + + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" ) var ( From e6429826292068170469cc05aae97a2ec0198d3f Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Tue, 31 Dec 2019 18:37:25 +0100 Subject: [PATCH 16/29] Adjust APIs to handle multiple related CRs --- .../servicebindingrequest/planner_test.go | 7 +- .../servicebindingrequest/retriever.go | 86 +++++++++---------- .../servicebindingrequest/retriever_test.go | 26 +++--- .../servicebindingrequest/secret_test.go | 6 +- 4 files changed, 63 insertions(+), 62 deletions(-) diff --git a/pkg/controller/servicebindingrequest/planner_test.go b/pkg/controller/servicebindingrequest/planner_test.go index d47934d0c2..cea2693d66 100644 --- a/pkg/controller/servicebindingrequest/planner_test.go +++ b/pkg/controller/servicebindingrequest/planner_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" "github.com/redhat-developer/service-binding-operator/test/mocks" ) @@ -26,6 +27,9 @@ func TestPlannerNew(t *testing.T) { } f := mocks.NewFake(t, ns) sbr := f.AddMockedServiceBindingRequest(name, resourceRef, "", deploymentsGVR, matchLabels) + sbr.Spec.BackingServiceSelectors = []v1alpha1.BackingServiceSelector{ + sbr.Spec.BackingServiceSelector, + } f.AddMockedUnstructuredCSV("cluster-service-version") f.AddMockedDatabaseCR(resourceRef) f.AddMockedUnstructuredDatabaseCRD() @@ -45,8 +49,7 @@ func TestPlannerNew(t *testing.T) { require.NoError(t, err) require.NotNil(t, plan) - require.NotNil(t, plan.CRDDescription) - require.NotNil(t, plan.CR) + require.NotEmpty(t, plan.RelatedResources) require.Equal(t, ns, plan.Ns) require.Equal(t, name, plan.Name) }) diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go index 744d7b3dc9..b75164ba26 100644 --- a/pkg/controller/servicebindingrequest/retriever.go +++ b/pkg/controller/servicebindingrequest/retriever.go @@ -53,9 +53,9 @@ func (r *Retriever) getNestedValue(key string, sectionMap interface{}) (string, } // getCRKey retrieve key in section from CR object, part of the "plan" instance. -func (r *Retriever) getCRKey(section string, key string) (string, interface{}, error) { - obj := r.plan.CR.Object - objName := r.plan.CR.GetName() +func (r *Retriever) getCRKey(u *unstructured.Unstructured, section string, key string) (string, interface{}, error) { + obj := u.Object + objName := u.GetName() log := r.logger.WithValues("CR.Name", objName, "CR.section", section, "CR.key", key) log.Debug("Reading CR attributes...") @@ -78,7 +78,7 @@ func (r *Retriever) getCRKey(section string, key string) (string, interface{}, e // read attributes from CR, where place means which top level key name contains the "path" actual // value, and parsing x-descriptors in order to either directly read CR data, or read items from // a secret. -func (r *Retriever) read(place, path string, xDescriptors []string) error { +func (r *Retriever) read(cr *unstructured.Unstructured, place, path string, xDescriptors []string) error { log := r.logger.WithValues( "CR.Section", place, "CRDDescription.Path", path, @@ -91,13 +91,13 @@ func (r *Retriever) read(place, path string, xDescriptors []string) error { // holds the configMap name and items configMaps := make(map[string][]string) - pathValue, _, err := r.getCRKey(place, path) + pathValue, _, err := r.getCRKey(cr, place, path) + if err != nil { + return err + } for _, xDescriptor := range xDescriptors { log = log.WithValues("CRDDescription.xDescriptor", xDescriptor, "cache", r.cache) log.Debug("Inspecting xDescriptor...") - if err != nil { - return err - } if _, ok := r.cache[place].(map[string]interface{}); !ok { r.cache[place] = make(map[string]interface{}) @@ -116,7 +116,7 @@ func (r *Retriever) read(place, path string, xDescriptors []string) error { r.markVisitedPaths(r.extractSecretItemName(xDescriptor), pathValue, place) r.VolumeKeys = append(r.VolumeKeys, pathValue) } else if strings.HasPrefix(xDescriptor, attributePrefix) { - r.store(path, []byte(pathValue)) + r.store(cr, path, []byte(pathValue)) } else { log.Debug("Defaulting....") } @@ -124,14 +124,14 @@ func (r *Retriever) read(place, path string, xDescriptors []string) error { for name, items := range secrets { // loading secret items all-at-once - err := r.readSecret(name, items, place, path) + err := r.readSecret(cr, name, items, place, path) if err != nil { return err } } for name, items := range configMaps { // add the function readConfigMap - err := r.readConfigMap(name, items, place, path) + err := r.readConfigMap(cr, name, items, place, path) if err != nil { return err } @@ -169,21 +169,17 @@ func (r *Retriever) markVisitedPaths(name, keyPath, fromPath string) { // readSecret based in secret name and list of items, read a secret from the same namespace informed // in plan instance. -func (r *Retriever) readSecret( - name string, - items []string, - fromPath string, - path string) error { +func (r *Retriever) readSecret(cr *unstructured.Unstructured, name string, items []string, fromPath string, path string) error { log := r.logger.WithValues("Secret.Name", name, "Secret.Items", items) log.Debug("Reading secret items...") gvr := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "secrets"} - u, err := r.client.Resource(gvr).Namespace(r.plan.Ns).Get(name, metav1.GetOptions{}) + secret, err := r.client.Resource(gvr).Namespace(r.plan.Ns).Get(name, metav1.GetOptions{}) if err != nil { return err } - data, exists, err := unstructured.NestedMap(u.Object, []string{"data"}...) + data, exists, err := unstructured.NestedMap(secret.Object, []string{"data"}...) if err != nil { return err } @@ -203,21 +199,17 @@ func (r *Retriever) readSecret( // update cache after reading configmap/secret in cache r.cache[fromPath].(map[string]interface{})[path].(map[string]interface{})[k] = string(data) // making sure key name has a secret reference - r.store(fmt.Sprintf("configMap_%s", k), data) - r.store(fmt.Sprintf("secret_%s", k), data) + r.store(cr, fmt.Sprintf("configMap_%s", k), data) + r.store(cr, fmt.Sprintf("secret_%s", k), data) } - r.Objects = append(r.Objects, u) + r.Objects = append(r.Objects, secret) return nil } // readConfigMap based in configMap name and list of items, read a configMap from the same namespace informed // in plan instance. -func (r *Retriever) readConfigMap( - name string, - items []string, - fromPath string, - path string) error { +func (r *Retriever) readConfigMap(cr *unstructured.Unstructured, name string, items []string, fromPath string, path string) error { log := r.logger.WithValues("ConfigMap.Name", name, "ConfigMap.Items", items) log.Debug("Reading ConfigMap items...") @@ -246,7 +238,7 @@ func (r *Retriever) readConfigMap( // update cache after reading configmap/secret in cache r.cache[fromPath].(map[string]interface{})[path].(map[string]interface{})[k] = value // making sure key name has a configMap reference - r.store(fmt.Sprintf("configMap_%s", k), []byte(value)) + r.store(cr, fmt.Sprintf("configMap_%s", k), []byte(value)) } r.Objects = append(r.Objects, u) @@ -254,13 +246,13 @@ func (r *Retriever) readConfigMap( } // store key and value, formatting key to look like an environment variable. -func (r *Retriever) store(key string, value []byte) { +func (r *Retriever) store(u *unstructured.Unstructured, key string, value []byte) { key = strings.ReplaceAll(key, ":", "_") key = strings.ReplaceAll(key, ".", "_") if r.bindingPrefix == "" { - key = fmt.Sprintf("%s_%s", r.plan.CR.GetKind(), key) + key = fmt.Sprintf("%s_%s", u.GetKind(), key) } else { - key = fmt.Sprintf("%s_%s_%s", r.bindingPrefix, r.plan.CR.GetKind(), key) + key = fmt.Sprintf("%s_%s_%s", r.bindingPrefix, u.GetKind(), key) } key = strings.ToUpper(key) r.data[key] = value @@ -275,39 +267,41 @@ func (r *Retriever) Retrieve() (map[string][]byte, error) { // ReadBindableResourcesData reads all related resources of a given sbr func (r *Retriever) ReadBindableResourcesData( sbr *v1alpha1.ServiceBindingRequest, - cr *unstructured.Unstructured, + crs []*unstructured.Unstructured, resources []schema.GroupVersionResource, ) error { r.logger.Info("Detecting extra resources for binding...") - b := NewDetectBindableResources(sbr, cr, []schema.GroupVersionResource{ - {Group: "", Version: "v1", Resource: "configmaps"}, - {Group: "", Version: "v1", Resource: "services"}, - {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, - }, r.client) - - vals, err := b.GetBindableVariables() - if err != nil { - return err - } - for k, v := range vals { - r.store(k, []byte(fmt.Sprintf("%v", v))) + for _, cr := range crs { + b := NewDetectBindableResources(sbr, cr, []schema.GroupVersionResource{ + {Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "", Version: "v1", Resource: "services"}, + {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, + }, r.client) + + vals, err := b.GetBindableVariables() + if err != nil { + return err + } + for k, v := range vals { + r.store(cr, k, []byte(fmt.Sprintf("%v", v))) + } } return nil } // ReadCRDDescriptionData reads data related to given crdDescription -func (r *Retriever) ReadCRDDescriptionData(crdDescription *olmv1alpha1.CRDDescription) error { +func (r *Retriever) ReadCRDDescriptionData(u *unstructured.Unstructured, crdDescription *olmv1alpha1.CRDDescription) error { r.logger.Info("Looking for spec-descriptors in 'spec'...") for _, specDescriptor := range crdDescription.SpecDescriptors { - if err := r.read("spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { + if err := r.read(u, "spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { return err } } r.logger.Info("Looking for status-descriptors in 'status'...") for _, statusDescriptor := range crdDescription.StatusDescriptors { - if err := r.read("status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { + if err := r.read(u, "status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { return err } } diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go index bd16bf4652..3ea58315bc 100644 --- a/pkg/controller/servicebindingrequest/retriever_test.go +++ b/pkg/controller/servicebindingrequest/retriever_test.go @@ -33,7 +33,7 @@ func TestRetriever(t *testing.T) { require.NotNil(t, retriever) t.Run("retrieve", func(t *testing.T) { - _ = retriever.ReadCRDDescriptionData(&crdDescription) + _ = retriever.ReadCRDDescriptionData(cr, &crdDescription) objs, err := retriever.Retrieve() require.NoError(t, err) require.NotEmpty(t, retriever.data) @@ -41,14 +41,14 @@ func TestRetriever(t *testing.T) { }) t.Run("getCRKey", func(t *testing.T) { - imageName, _, err := retriever.getCRKey("spec", "imageName") + imageName, _, err := retriever.getCRKey(cr, "spec", "imageName") require.NoError(t, err) require.Equal(t, "postgres", imageName) }) t.Run("read", func(t *testing.T) { // reading from secret, from status attribute - err := retriever.read("status", "dbCredentials", []string{ + err := retriever.read(cr, "status", "dbCredentials", []string{ "binding:env:object:secret:user", "binding:env:object:secret:password", }) @@ -59,7 +59,7 @@ func TestRetriever(t *testing.T) { require.Contains(t, retriever.data, "SERVICE_BINDING_DATABASE_SECRET_PASSWORD") // reading from spec attribute - err = retriever.read("spec", "image", []string{ + err = retriever.read(cr, "spec", "image", []string{ "binding:env:attribute", }) require.NoError(t, err) @@ -77,7 +77,7 @@ func TestRetriever(t *testing.T) { t.Run("readSecret", func(t *testing.T) { retriever.data = make(map[string][]byte) - err := retriever.readSecret("db-credentials", []string{"user", "password"}, "spec", "dbConfigMap") + err := retriever.readSecret(cr, "db-credentials", []string{"user", "password"}, "spec", "dbConfigMap") require.NoError(t, err) require.Contains(t, retriever.data, "SERVICE_BINDING_DATABASE_SECRET_USER") @@ -85,7 +85,7 @@ func TestRetriever(t *testing.T) { }) t.Run("store", func(t *testing.T) { - retriever.store("test", []byte("test")) + retriever.store(cr, "test", []byte("test")) require.Contains(t, retriever.data, "SERVICE_BINDING_DATABASE_TEST") require.Equal(t, []byte("test"), retriever.data["SERVICE_BINDING_DATABASE_TEST"]) }) @@ -95,7 +95,7 @@ func TestRetriever(t *testing.T) { require.NotNil(t, retriever) retriever.data = make(map[string][]byte) - err := retriever.readSecret("db-credentials", []string{"user", "password"}, "spec", "dbConfigMap") + err := retriever.readSecret(cr, "db-credentials", []string{"user", "password"}, "spec", "dbConfigMap") require.NoError(t, err) require.Contains(t, retriever.data, "DATABASE_SECRET_USER") @@ -126,7 +126,7 @@ func TestRetrieverWithNestedCRKey(t *testing.T) { require.NotNil(t, retriever) t.Run("Second level", func(t *testing.T) { - imageName, _, err := retriever.getCRKey("spec", "image.name") + imageName, _, err := retriever.getCRKey(cr, "spec", "image.name") require.NoError(t, err) require.Equal(t, "postgres", imageName) }) @@ -134,12 +134,12 @@ func TestRetrieverWithNestedCRKey(t *testing.T) { t.Run("Second level error", func(t *testing.T) { // FIXME: if attribute isn't available in CR we would not throw any error. t.Skip() - _, _, err := retriever.getCRKey("spec", "image..name") + _, _, err := retriever.getCRKey(cr, "spec", "image..name") require.NotNil(t, err) }) t.Run("Third level", func(t *testing.T) { - something, _, err := retriever.getCRKey("spec", "image.third.something") + something, _, err := retriever.getCRKey(cr, "spec", "image.third.something") require.NoError(t, err) require.Equal(t, "somevalue", something) }) @@ -172,7 +172,7 @@ func TestRetrieverWithConfigMap(t *testing.T) { t.Run("read", func(t *testing.T) { // reading from configMap, from status attribute - err = retriever.read("spec", "dbConfigMap", []string{ + err = retriever.read(cr, "spec", "dbConfigMap", []string{ "binding:env:object:configmap:user", "binding:env:object:configmap:password", }) @@ -191,7 +191,7 @@ func TestRetrieverWithConfigMap(t *testing.T) { t.Run("readConfigMap", func(t *testing.T) { retriever.data = make(map[string][]byte) - err := retriever.readConfigMap(crName, []string{"user", "password"}, "spec", "dbConfigMap") + err := retriever.readConfigMap(cr, crName, []string{"user", "password"}, "spec", "dbConfigMap") require.NoError(t, err) require.Contains(t, retriever.data, ("SERVICE_BINDING_DATABASE_CONFIGMAP_USER")) @@ -223,7 +223,7 @@ func TestCustomEnvParser(t *testing.T) { require.NotNil(t, retriever) t.Run("Should detect custom env values", func(t *testing.T) { - _ = retriever.ReadCRDDescriptionData(&crdDescription) + _ = retriever.ReadCRDDescriptionData(cr, &crdDescription) _, err = retriever.Retrieve() require.NoError(t, err) diff --git a/pkg/controller/servicebindingrequest/secret_test.go b/pkg/controller/servicebindingrequest/secret_test.go index 207e210505..6d6db8adc5 100644 --- a/pkg/controller/servicebindingrequest/secret_test.go +++ b/pkg/controller/servicebindingrequest/secret_test.go @@ -23,7 +23,11 @@ func TestSecretNew(t *testing.T) { matchLabels := map[string]string{} sbr := mocks.ServiceBindingRequestMock(ns, name, "", "", deploymentsGVR, matchLabels, true) - plan := &Plan{Ns: ns, Name: name, CRDDescription: nil, SBR: *sbr} + plan := &Plan{ + Ns: ns, + Name: name, + SBR: *sbr, + } data := map[string][]byte{"key": []byte("value")} s := NewSecret(f.FakeDynClient(), plan) From 7923e8af406d728e553f3c1a426fd9d5dab72c36 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Tue, 31 Dec 2019 18:40:55 +0100 Subject: [PATCH 17/29] Group CRD and CR into RelatedResources --- .../servicebindingrequest/binding.go | 17 ++-- .../servicebindingrequest/planner.go | 89 +++++++++++++------ .../servicebindingrequest/retriever_test.go | 44 ++++++++- 3 files changed, 109 insertions(+), 41 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index b0a41dc3e4..2c87e82a98 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -282,10 +282,9 @@ func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error return nil, err } - if plan.CR != nil { - // append only if contains a value - objs = append(objs, plan.CR) - } + rs := plan.RelatedResources.GetCRs() + // append all SBR related CRs + objs = append(objs, rs...) // retriever is responsible for gathering data related to the given plan. retriever := NewRetriever(options.DynClient, plan, options.EnvVarPrefix) @@ -293,7 +292,7 @@ func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error // read bindable data from the specified resources if options.DetectBindingResources { err := retriever.ReadBindableResourcesData( - &plan.SBR, plan.CR, []schema.GroupVersionResource{ + &plan.SBR, rs, []schema.GroupVersionResource{ {Group: "", Version: "v1", Resource: "configmaps"}, {Group: "", Version: "v1", Resource: "services"}, {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, @@ -305,9 +304,11 @@ func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error } // read bindable data from the CRDDescription found by the planner - err = retriever.ReadCRDDescriptionData(plan.CRDDescription) - if err != nil { - return nil, err + for _, r := range plan.RelatedResources { + err = retriever.ReadCRDDescriptionData(r.CR, r.CRDDescription) + if err != nil { + return nil, err + } } // gather retriever's read data diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go index e0b21313db..b476ce642c 100644 --- a/pkg/controller/servicebindingrequest/planner.go +++ b/pkg/controller/servicebindingrequest/planner.go @@ -29,11 +29,10 @@ type Planner struct { // Plan outcome, after executing planner. type Plan struct { - Ns string // namespace name - Name string // plan name, same than ServiceBindingRequest - CRDDescription *olmv1alpha1.CRDDescription // custom resource definition description - CR *unstructured.Unstructured // custom resource object - SBR v1alpha1.ServiceBindingRequest // service binding request + Ns string // namespace name + Name string // plan name, same than ServiceBindingRequest + SBR v1alpha1.ServiceBindingRequest // service binding request + RelatedResources RelatedResources // CR and CRDDescription pairs SBR related } // searchCR based on a CustomResourceDefinitionDescription and name, search for the object. @@ -61,38 +60,70 @@ func (p *Planner) searchCRD(gvk schema.GroupVersionKind) (*unstructured.Unstruct return p.client.Resource(CRDGVR).Get(crdName, metav1.GetOptions{}) } +// RelatedResource represents a SBR related resource, composed by its CR and CRDDescription. +type RelatedResource struct { + CRDDescription *olmv1alpha1.CRDDescription + CR *unstructured.Unstructured +} + +// RelatedResources contains a collection of SBR related resources. +type RelatedResources []*RelatedResource + +// GetCRs returns a slice of unstructured CRs contained in the collection. +func (rr RelatedResources) GetCRs() []*unstructured.Unstructured { + var crs []*unstructured.Unstructured + for _, r := range rr { + crs = append(crs, r.CR) + } + return crs +} + // Plan by retrieving the necessary resources related to binding a service backend. func (p *Planner) Plan() (*Plan, error) { - bssGVK := p.sbr.Spec.BackingServiceSelector.GroupVersionKind + ns := p.sbr.GetNamespace() + selector := p.sbr.Spec.BackingServiceSelector + selectors := append( + []v1alpha1.BackingServiceSelector{selector}, + p.sbr.Spec.BackingServiceSelectors..., + ) - // resolve the CRD using the service's GVK - crd, err := p.searchCRD(bssGVK) - if err != nil { - return nil, err - } - p.logger.Debug("Resolved CRD", "CRD", crd) + relatedResources := make([]*RelatedResource, 0, 0) + for _, s := range selectors { + bssGVK := s.GroupVersionKind - // resolve the CRDDescription based on the service's GVK and the resolved CRD - olm := NewOLM(p.client, p.sbr.GetNamespace()) - crdDescription, err := olm.SelectCRDByGVK(bssGVK, crd) - if err != nil { - return nil, err - } - p.logger.Debug("Resolved CRDDescription", "CRDDescription", crdDescription) + // resolve the CRD using the service's GVK + crd, err := p.searchCRD(bssGVK) + if err != nil { + return nil, err + } + p.logger.Debug("Resolved CRD", "CRD", crd) + + // resolve the CRDDescription based on the service's GVK and the resolved CRD + olm := NewOLM(p.client, ns) + crdDescription, err := olm.SelectCRDByGVK(bssGVK, crd) + if err != nil { + return nil, err + } + p.logger.Debug("Resolved CRDDescription", "CRDDescription", crdDescription) + + cr, err := p.searchCR(ns, s) + if err != nil { + return nil, err + } - // retrieve the CR referred by the service - cr, err := p.searchCR(p.sbr.GetNamespace(), p.sbr.Spec.BackingServiceSelector) - if err != nil { - return nil, err + r := &RelatedResource{ + CRDDescription: crdDescription, + CR: cr, + } + relatedResources = append(relatedResources, r) + p.logger.Debug("Resolved related resource", "RelatedResource", r) } - p.logger.Debug("Resolved CR", "CR", cr) return &Plan{ - Ns: p.sbr.GetNamespace(), - Name: p.sbr.GetName(), - CRDDescription: crdDescription, - CR: cr, - SBR: *p.sbr, + Name: p.sbr.GetName(), + Ns: ns, + RelatedResources: relatedResources, + SBR: *p.sbr, }, nil } diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go index 3ea58315bc..a67a54b53d 100644 --- a/pkg/controller/servicebindingrequest/retriever_test.go +++ b/pkg/controller/servicebindingrequest/retriever_test.go @@ -25,7 +25,16 @@ func TestRetriever(t *testing.T) { cr, err := mocks.UnstructuredDatabaseCRMock(ns, crName) require.NoError(t, err) - plan := &Plan{Ns: ns, Name: "retriever", CRDDescription: &crdDescription, CR: cr} + plan := &Plan{ + Ns: ns, + Name: "retriever", + RelatedResources: []*RelatedResource{ + { + CRDDescription: &crdDescription, + CR: cr, + }, + }, + } fakeDynClient := f.FakeDynClient() @@ -118,7 +127,16 @@ func TestRetrieverWithNestedCRKey(t *testing.T) { cr, err := mocks.UnstructuredNestedDatabaseCRMock(ns, crName) require.NoError(t, err) - plan := &Plan{Ns: ns, Name: "retriever", CRDDescription: &crdDescription, CR: cr} + plan := &Plan{ + Ns: ns, + Name: "retriever", + RelatedResources: []*RelatedResource{ + { + CRDDescription: &crdDescription, + CR: cr, + }, + }, + } fakeDynClient := f.FakeDynClient() @@ -163,7 +181,16 @@ func TestRetrieverWithConfigMap(t *testing.T) { cr, err := mocks.UnstructuredDatabaseConfigMapMock(ns, crName, crName) require.NoError(t, err) - plan := &Plan{Ns: ns, Name: "retriever", CRDDescription: &crdDescription, CR: cr} + plan := &Plan{ + Ns: ns, + Name: "retriever", + RelatedResources: []*RelatedResource{ + { + CRDDescription: &crdDescription, + CR: cr, + }, + }, + } fakeDynClient := f.FakeDynClient() @@ -215,7 +242,16 @@ func TestCustomEnvParser(t *testing.T) { cr, err := mocks.UnstructuredDatabaseCRMock(ns, crName) require.NoError(t, err) - plan := &Plan{Ns: ns, Name: "retriever", CRDDescription: &crdDescription, CR: cr} + plan := &Plan{ + Ns: ns, + Name: "retriever", + RelatedResources: []*RelatedResource{ + { + CRDDescription: &crdDescription, + CR: cr, + }, + }, + } fakeDynClient := f.FakeDynClient() From ca56e237efb3e75a6c403eca8d1431e059a194cb Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 1 Jan 2020 16:41:25 +0100 Subject: [PATCH 18/29] Reorder method definitions --- .../servicebindingrequest/binding.go | 90 +++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index 2c87e82a98..9694fa83e4 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -78,6 +78,32 @@ func removeStringSlice(slice []string, str string) []string { return cleanSlice } +// updateServiceBindingRequest execute update API call on a SBR request. It can return errors from +// this action. +func (b *BindingManager) updateServiceBindingRequest( + sbr *v1alpha1.ServiceBindingRequest, +) (*v1alpha1.ServiceBindingRequest, error) { + u, err := converter.ToUnstructured(sbr) + if err != nil { + return nil, err + } + + u, err = b.DynClient. + Resource(GroupVersion). + Namespace(sbr.GetNamespace()). + Update(u, v1.UpdateOptions{}) + + if err != nil { + return nil, err + } + + err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) + if err != nil { + return nil, err + } + return sbr, nil +} + // Unbind removes the relationship between a Service Binding Request and its related objects. func (b *BindingManager) Unbind() (reconcile.Result, error) { logger := b.Logger.WithName("Unbind") @@ -108,21 +134,31 @@ func (b *BindingManager) Unbind() (reconcile.Result, error) { return Done() } -// updateServiceBindingRequest execute update API call on a SBR request. It can return errors from -// this action. -func (b *BindingManager) updateServiceBindingRequest( +// updateStatusServiceBindingRequest updates the Service Binding Request's status field. +func (b *BindingManager) updateStatusServiceBindingRequest( sbr *v1alpha1.ServiceBindingRequest, -) (*v1alpha1.ServiceBindingRequest, error) { + sbrStatus *v1alpha1.ServiceBindingRequestStatus, +) ( + *v1alpha1.ServiceBindingRequest, + error, +) { + // do not update if both statuses are equal + if result := cmp.DeepEqual(sbr.Status, sbrStatus)(); result.Success() { + return sbr, nil + } + + // coping status over informed object + sbr.Status = *sbrStatus + + // converting object into unstructured u, err := converter.ToUnstructured(sbr) if err != nil { return nil, err } - u, err = b.DynClient. - Resource(GroupVersion). - Namespace(sbr.GetNamespace()). - Update(u, v1.UpdateOptions{}) - + gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) + resourceClient := b.DynClient.Resource(gr).Namespace(sbr.GetNamespace()) + u, err = resourceClient.UpdateStatus(u, v1.UpdateOptions{}) if err != nil { return nil, err } @@ -205,42 +241,6 @@ func (b *BindingManager) Bind() (reconcile.Result, error) { return Done() } -// updateStatusServiceBindingRequest updates the Service Binding Request's status field. -func (b *BindingManager) updateStatusServiceBindingRequest( - sbr *v1alpha1.ServiceBindingRequest, - sbrStatus *v1alpha1.ServiceBindingRequestStatus, -) ( - *v1alpha1.ServiceBindingRequest, - error, -) { - // do not update if both statuses are equal - if result := cmp.DeepEqual(sbr.Status, sbrStatus)(); result.Success() { - return sbr, nil - } - - // coping status over informed object - sbr.Status = *sbrStatus - - // converting object into unstructured - u, err := converter.ToUnstructured(sbr) - if err != nil { - return nil, err - } - - gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) - resourceClient := b.DynClient.Resource(gr).Namespace(sbr.GetNamespace()) - u, err = resourceClient.UpdateStatus(u, v1.UpdateOptions{}) - if err != nil { - return nil, err - } - - err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr) - if err != nil { - return nil, err - } - return sbr, nil -} - // setApplicationObjects replaces the Status's equivalent field. func (b *BindingManager) setApplicationObjects( sbrStatus *v1alpha1.ServiceBindingRequestStatus, From cbe927e44d9d74c1914a855cba1d297667cc3da7 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 1 Jan 2020 16:48:59 +0100 Subject: [PATCH 19/29] Rename BindingManager to ServiceBinder --- .../servicebindingrequest/binding.go | 30 +++++++++---------- .../servicebindingrequest/reconciler.go | 8 ++--- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index 9694fa83e4..20819075ca 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -34,8 +34,8 @@ const ( // GroupVersion represents the service binding request resource's group version. var GroupVersion = v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource) -// BindingManagerOptions is BuildBindingManager arguments. -type BindingManagerOptions struct { +// ServiceBinderOptions is BuildServiceBinder arguments. +type ServiceBinderOptions struct { Logger *log.Log DynClient dynamic.Interface DetectBindingResources bool @@ -45,12 +45,12 @@ type BindingManagerOptions struct { } // Valid returns whether the options are valid. -func (o *BindingManagerOptions) Valid() bool { +func (o *ServiceBinderOptions) Valid() bool { return o.SBR != nil && o.DynClient != nil && o.Client != nil } -// BindingManager manages binding for a Service Binding Request and associated objects. -type BindingManager struct { +// ServiceBinder manages binding for a Service Binding Request and associated objects. +type ServiceBinder struct { // Binder is responsible for interacting with the cluster and apply binding related changes. Binder *Binder // Data is the collection of all data read by the manager. @@ -80,7 +80,7 @@ func removeStringSlice(slice []string, str string) []string { // updateServiceBindingRequest execute update API call on a SBR request. It can return errors from // this action. -func (b *BindingManager) updateServiceBindingRequest( +func (b *ServiceBinder) updateServiceBindingRequest( sbr *v1alpha1.ServiceBindingRequest, ) (*v1alpha1.ServiceBindingRequest, error) { u, err := converter.ToUnstructured(sbr) @@ -105,7 +105,7 @@ func (b *BindingManager) updateServiceBindingRequest( } // Unbind removes the relationship between a Service Binding Request and its related objects. -func (b *BindingManager) Unbind() (reconcile.Result, error) { +func (b *ServiceBinder) Unbind() (reconcile.Result, error) { logger := b.Logger.WithName("Unbind") logger.Info("Cleaning related objects from operator's annotations...") @@ -135,7 +135,7 @@ func (b *BindingManager) Unbind() (reconcile.Result, error) { } // updateStatusServiceBindingRequest updates the Service Binding Request's status field. -func (b *BindingManager) updateStatusServiceBindingRequest( +func (b *ServiceBinder) updateStatusServiceBindingRequest( sbr *v1alpha1.ServiceBindingRequest, sbrStatus *v1alpha1.ServiceBindingRequestStatus, ) ( @@ -172,7 +172,7 @@ func (b *BindingManager) updateStatusServiceBindingRequest( // onError comprise the update of ServiceBindingRequest status to set error flag, and inspect // informed error to apply a different behavior for not-founds. -func (b *BindingManager) onError( +func (b *ServiceBinder) onError( err error, sbr *v1alpha1.ServiceBindingRequest, sbrStatus *v1alpha1.ServiceBindingRequestStatus, @@ -194,7 +194,7 @@ func (b *BindingManager) onError( } // Bind configures binding between the Service Binding Request and its related objects. -func (b *BindingManager) Bind() (reconcile.Result, error) { +func (b *ServiceBinder) Bind() (reconcile.Result, error) { sbrStatus := b.SBR.Status.DeepCopy() b.Logger.Info("Saving data on intermediary secret...") @@ -242,7 +242,7 @@ func (b *BindingManager) Bind() (reconcile.Result, error) { } // setApplicationObjects replaces the Status's equivalent field. -func (b *BindingManager) setApplicationObjects( +func (b *ServiceBinder) setApplicationObjects( sbrStatus *v1alpha1.ServiceBindingRequestStatus, objs []*unstructured.Unstructured, ) { @@ -263,11 +263,11 @@ func buildPlan( return planner.Plan() } -// InvalidOptionsErr is returned when BindingManagerOptions are not valid. +// InvalidOptionsErr is returned when ServiceBinderOptions are not valid. var InvalidOptionsErr = errors.New("invalid options") -// BuildBindingManager creates a new binding manager according to options. -func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error) { +// BuildServiceBinder creates a new binding manager according to options. +func BuildServiceBinder(options *ServiceBinderOptions) (*ServiceBinder, error) { if !options.Valid() { return nil, InvalidOptionsErr } @@ -325,7 +325,7 @@ func BuildBindingManager(options *BindingManagerOptions) (*BindingManager, error objs = append(objs, secretObj) } - return &BindingManager{ + return &ServiceBinder{ Logger: options.Logger, Binder: NewBinder(ctx, options.Client, options.DynClient, options.SBR, retriever.VolumeKeys), DynClient: options.DynClient, diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go index cc8a6d1cb3..e29a50ffad 100644 --- a/pkg/controller/servicebindingrequest/reconciler.go +++ b/pkg/controller/servicebindingrequest/reconciler.go @@ -64,7 +64,7 @@ func (r *Reconciler) getServiceBindingRequest( // unbind removes the relationship between the given sbr and the manifests the operator has // previously modified. This process also deletes any manifests created to support the binding // functionality, such as ConfigMaps and Secrets. -func (r *Reconciler) unbind(logger *log.Log, bm *BindingManager) (reconcile.Result, error) { +func (r *Reconciler) unbind(logger *log.Log, bm *ServiceBinder) (reconcile.Result, error) { logger = logger.WithName("unbind") // when finalizer is not found anymore, it can be safely removed @@ -87,7 +87,7 @@ func (r *Reconciler) unbind(logger *log.Log, bm *BindingManager) (reconcile.Resu // in the common parts of the reconciler, and execute the final binding steps. func (r *Reconciler) bind( logger *log.Log, - bm *BindingManager, + bm *ServiceBinder, sbrStatus *v1alpha1.ServiceBindingRequestStatus, ) ( reconcile.Result, @@ -130,7 +130,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // splitting instance from it's status sbrStatus := &sbr.Status - options := &BindingManagerOptions{ + options := &ServiceBinderOptions{ Client: r.client, DynClient: r.dynClient, DetectBindingResources: sbr.Spec.DetectBindingResources, @@ -139,7 +139,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err Logger: logger, } - bm, err := BuildBindingManager(options) + bm, err := BuildServiceBinder(options) if err != nil { logger.Error(err, "Creating binding context") return RequeueError(err) From a280ee9436c79e87c06aaae658cf4a4fd811406f Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 1 Jan 2020 17:56:59 +0100 Subject: [PATCH 20/29] Return CR after adding it to known objects --- test/mocks/fake.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/mocks/fake.go b/test/mocks/fake.go index 3aea10a97c..35988285a7 100644 --- a/test/mocks/fake.go +++ b/test/mocks/fake.go @@ -148,10 +148,11 @@ func (f *Fake) AddMockedUnstructuredDatabaseCRD() *unstructured.Unstructured { return c } -func (f *Fake) AddMockedUnstructuredPostgresDatabaseCR(ref string) { +func (f *Fake) AddMockedUnstructuredPostgresDatabaseCR(ref string) *unstructured.Unstructured { d, err := UnstructuredPostgresDatabaseCRMock(f.ns, ref) require.NoError(f.t, err) f.objs = append(f.objs, d) + return d } // AddMockedSecret add mocked object from SecretMock. From 7b11f10cf80bb0efd0052bde3a0042e0ade9d781 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Wed, 1 Jan 2020 18:03:38 +0100 Subject: [PATCH 21/29] Return unstructured Deployment --- test/mocks/fake.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/mocks/fake.go b/test/mocks/fake.go index 35988285a7..0316455cf4 100644 --- a/test/mocks/fake.go +++ b/test/mocks/fake.go @@ -131,12 +131,13 @@ func (f *Fake) AddMockedUnstructuredDeploymentConfig(name string, matchLabels ma } // AddMockedUnstructuredDeployment add mocked object from UnstructuredDeploymentMock. -func (f *Fake) AddMockedUnstructuredDeployment(name string, matchLabels map[string]string) { +func (f *Fake) AddMockedUnstructuredDeployment(name string, matchLabels map[string]string) *unstructured.Unstructured { require.NoError(f.t, appsv1.AddToScheme(f.S)) d, err := UnstructuredDeploymentMock(f.ns, name, matchLabels) require.NoError(f.t, err) f.S.AddKnownTypes(appsv1.SchemeGroupVersion, &appsv1.Deployment{}) f.objs = append(f.objs, d) + return d } func (f *Fake) AddMockedUnstructuredDatabaseCRD() *unstructured.Unstructured { From cc3cb7a7f42f13601b56dea770be9f55b5ce3089 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 2 Jan 2020 10:02:01 +0100 Subject: [PATCH 22/29] Fix an issue where selector could be empty, leading to a nil element in selectors --- pkg/controller/servicebindingrequest/planner.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go index b476ce642c..991038939a 100644 --- a/pkg/controller/servicebindingrequest/planner.go +++ b/pkg/controller/servicebindingrequest/planner.go @@ -81,11 +81,11 @@ func (rr RelatedResources) GetCRs() []*unstructured.Unstructured { // Plan by retrieving the necessary resources related to binding a service backend. func (p *Planner) Plan() (*Plan, error) { ns := p.sbr.GetNamespace() + selectors := append([]v1alpha1.BackingServiceSelector{}, p.sbr.Spec.BackingServiceSelectors...) selector := p.sbr.Spec.BackingServiceSelector - selectors := append( - []v1alpha1.BackingServiceSelector{selector}, - p.sbr.Spec.BackingServiceSelectors..., - ) + if len(selector.ResourceRef) > 0 { + selectors = append(selectors, selector) + } relatedResources := make([]*RelatedResource, 0, 0) for _, s := range selectors { From 35a37e5aea2a973a8a333d4eff77de8bea1680c7 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 2 Jan 2020 13:19:51 +0100 Subject: [PATCH 23/29] Change return type to expose actions collected by FakeDynamicClient --- test/mocks/fake.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/mocks/fake.go b/test/mocks/fake.go index 0316455cf4..44ea7cecac 100644 --- a/test/mocks/fake.go +++ b/test/mocks/fake.go @@ -12,13 +12,13 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/dynamic" fakedynamic "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ocav1 "github.com/openshift/api/apps/v1" + v1alpha1 "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" ) @@ -176,7 +176,7 @@ func (f *Fake) FakeClient() client.Client { } // FakeDynClient returns fake dynamic api client. -func (f *Fake) FakeDynClient() dynamic.Interface { +func (f *Fake) FakeDynClient() *fakedynamic.FakeDynamicClient { return fakedynamic.NewSimpleDynamicClient(f.S, f.objs...) } From 0344b4b8963af695c25b112f640836a09d363a21 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 2 Jan 2020 13:38:21 +0100 Subject: [PATCH 24/29] Convert v1.ConfigMap to Unstructured before adding it to test fixtures --- pkg/controller/servicebindingrequest/retriever_test.go | 2 +- test/mocks/fake.go | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go index a67a54b53d..8ca25c79fc 100644 --- a/pkg/controller/servicebindingrequest/retriever_test.go +++ b/pkg/controller/servicebindingrequest/retriever_test.go @@ -173,7 +173,7 @@ func TestRetrieverWithConfigMap(t *testing.T) { f := mocks.NewFake(t, ns) f.AddMockedUnstructuredCSV("csv") - f.AddMockedConfigMap(crName) + f.AddMockedUnstructuredConfigMap(crName) f.AddMockedDatabaseCR(crName) crdDescription := mocks.CRDDescriptionConfigMapMock() diff --git a/test/mocks/fake.go b/test/mocks/fake.go index 44ea7cecac..daf3aae679 100644 --- a/test/mocks/fake.go +++ b/test/mocks/fake.go @@ -161,9 +161,12 @@ func (f *Fake) AddMockedSecret(name string) { f.objs = append(f.objs, SecretMock(f.ns, name)) } -// AddMockedConfigMap add mocked object from ConfigMapMock. -func (f *Fake) AddMockedConfigMap(name string) { - f.objs = append(f.objs, ConfigMapMock(f.ns, name)) +// AddMockedUnstructuredConfigMap add mocked object from ConfigMapMock. +func (f *Fake) AddMockedUnstructuredConfigMap(name string) { + mock := ConfigMapMock(f.ns, name) + uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mock) + require.NoError(f.t, err) + f.objs = append(f.objs, &unstructured.Unstructured{Object: uObj}) } func (f *Fake) AddMockResource(resource runtime.Object) { From 296c544b1adebdb0b53eab8f8bbc8190b8fd1207 Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Thu, 2 Jan 2020 14:12:28 +0100 Subject: [PATCH 25/29] Implement ServiceBinder bind tests --- .../servicebindingrequest/binding_test.go | 212 ++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 pkg/controller/servicebindingrequest/binding_test.go diff --git a/pkg/controller/servicebindingrequest/binding_test.go b/pkg/controller/servicebindingrequest/binding_test.go new file mode 100644 index 0000000000..0fb4880be2 --- /dev/null +++ b/pkg/controller/servicebindingrequest/binding_test.go @@ -0,0 +1,212 @@ +package servicebindingrequest + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" + + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" + "github.com/redhat-developer/service-binding-operator/pkg/log" + "github.com/redhat-developer/service-binding-operator/test/mocks" +) + +// TestServiceBinder_Bind exercises scenarios regarding binding SBR and its related resources. +func TestServiceBinder_Bind(t *testing.T) { + // wantedAction represents an action issued by the component that is required to exist after it + // finished the operation + type wantedAction struct { + verb string + resource string + name string + } + + // args are the test arguments + type args struct { + // options inform the test how to build the ServiceBinder. + options *ServiceBinderOptions + // wantBuildErr informs the test an error is wanted at build phase. + wantBuildErr error + // wantErr informs the test an error is wanted at ServiceBinder's bind phase. + wantErr error + // wantedActions informs the test all the actions that should have been issued by + // ServiceBinder. + wantedActions []wantedAction + } + + // assertBind exercises the bind functionality + assertBind := func(args args) func(*testing.T) { + return func(t *testing.T) { + sb, err := BuildServiceBinder(args.options) + if args.wantBuildErr != nil { + require.Error(t, err) + return + } else { + require.NoError(t, err) + } + + res, err := sb.Bind() + + if args.wantErr != nil { + require.Error(t, err) + require.Equal(t, args.wantErr, err) + require.Nil(t, res) + } else { + require.NoError(t, err) + require.NotNil(t, res) + } + + // extract actions from the dynamic client, regardless of the bind status; it is expected + // that failures also issue updates for ServiceBindingRequest objects + dynClient, ok := sb.DynClient.(*fake.FakeDynamicClient) + require.True(t, ok) + actions := dynClient.Actions() + require.NotNil(t, actions) + + // regardless of the result, verify the actions expected by the reconciliation + // process have been issued if user has specified wanted actions + if len(args.wantedActions) > 0 { + // proceed to find whether actions match wanted actions + for _, w := range args.wantedActions { + var match bool + // search for each wanted action in the slice of actions issued by ServiceBinder + for _, a := range actions { + // match will be updated in the switch branches below + if match { + break + } + + if a.Matches(w.verb, w.resource) { + // there are several action types; here it is required to 'type + // switch' it and perform the right check. + switch v := a.(type) { + case k8stesting.GetAction: + match = v.GetName() == w.name + case k8stesting.UpdateAction: + obj := v.GetObject() + uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) + require.NoError(t, err) + u := &unstructured.Unstructured{Object: uObj} + match = w.name == u.GetName() + } + } + + // short circuit to the end of collected actions if the action has matched. + if match { + break + } + } + require.True(t, match, "expected action %+v not found", w) + } + } + } + } + + matchLabels := map[string]string{ + "connects-to": "database", + } + + f := mocks.NewFake(t, reconcilerName) + f.S.AddKnownTypes(v1alpha1.SchemeGroupVersion, &v1alpha1.ServiceBindingRequest{}) + f.S.AddKnownTypes(corev1.SchemeGroupVersion, &corev1.ConfigMap{}) + + d := f.AddMockedUnstructuredDeployment(reconcilerName, matchLabels) + f.AddMockedUnstructuredDatabaseCRD() + f.AddMockedUnstructuredConfigMap(reconcilerName) + + // create and munge a Database CR since there's no "Status" field in + // databases.postgres.baiju.dev, requiring us to add the field directly in the unstructured + // object + cr := f.AddMockedUnstructuredPostgresDatabaseCR(reconcilerName) + runtimeStatus := map[string]interface{}{ + "dbConfigMap": reconcilerName, + } + err := unstructured.SetNestedMap(cr.Object, runtimeStatus, "status") + require.NoError(t, err) + + // create the ServiceBindingRequest + sbr := &v1alpha1.ServiceBindingRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps.openshift.io/v1alpha1", + Kind: "ServiceBindingRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: reconcilerName, + }, + Spec: v1alpha1.ServiceBindingRequestSpec{ + ApplicationSelector: v1alpha1.ApplicationSelector{ + MatchLabels: matchLabels, + Group: d.GetObjectKind().GroupVersionKind().Group, + Version: d.GetObjectKind().GroupVersionKind().Version, + Resource: "deployments", + ResourceRef: reconcilerName, + }, + BackingServiceSelectors: []v1alpha1.BackingServiceSelector{ + { + GroupVersionKind: cr.GetObjectKind().GroupVersionKind(), + ResourceRef: reconcilerName, + }, + }, + }, + Status: v1alpha1.ServiceBindingRequestStatus{}, + } + f.AddMockResource(sbr) + + logger := log.NewLog("service-binder") + t.Run("bind golden path", assertBind(args{ + options: &ServiceBinderOptions{ + Logger: logger, + DynClient: f.FakeDynClient(), + DetectBindingResources: false, + EnvVarPrefix: "", + SBR: sbr, + Client: f.FakeClient(), + }, + wantedActions: []wantedAction{ + { + resource: "servicebindingrequests", + verb: "update", + name: reconcilerName, + }, + { + resource: "secrets", + verb: "update", + name: reconcilerName, + }, + { + resource: "databases", + verb: "update", + name: reconcilerName, + }, + }, + })) + + t.Run("bind with binding resource detection", assertBind(args{ + options: &ServiceBinderOptions{ + Logger: logger, + DynClient: f.FakeDynClient(), + DetectBindingResources: true, + EnvVarPrefix: "", + SBR: sbr, + Client: f.FakeClient(), + }, + })) + + // Missing SBR returns an InvalidOptionsErr + t.Run("bind missing SBR", assertBind(args{ + options: &ServiceBinderOptions{ + Logger: logger, + DynClient: f.FakeDynClient(), + DetectBindingResources: false, + EnvVarPrefix: "", + SBR: nil, + Client: f.FakeClient(), + }, + wantBuildErr: InvalidOptionsErr, + })) +} From bd1d3ed16b205527e3a4c2433652d488fab79bef Mon Sep 17 00:00:00 2001 From: Igor Sutton Date: Fri, 3 Jan 2020 13:31:58 +0100 Subject: [PATCH 26/29] Add support for multiple backing services Validation of the output secret is not yet being performed. --- .../servicebindingrequest/binding_test.go | 113 +++++++++++++++--- 1 file changed, 95 insertions(+), 18 deletions(-) diff --git a/pkg/controller/servicebindingrequest/binding_test.go b/pkg/controller/servicebindingrequest/binding_test.go index 0fb4880be2..0f2a622245 100644 --- a/pkg/controller/servicebindingrequest/binding_test.go +++ b/pkg/controller/servicebindingrequest/binding_test.go @@ -117,26 +117,66 @@ func TestServiceBinder_Bind(t *testing.T) { d := f.AddMockedUnstructuredDeployment(reconcilerName, matchLabels) f.AddMockedUnstructuredDatabaseCRD() - f.AddMockedUnstructuredConfigMap(reconcilerName) + f.AddMockedUnstructuredConfigMap("db1") + f.AddMockedUnstructuredConfigMap("db2") // create and munge a Database CR since there's no "Status" field in // databases.postgres.baiju.dev, requiring us to add the field directly in the unstructured // object - cr := f.AddMockedUnstructuredPostgresDatabaseCR(reconcilerName) - runtimeStatus := map[string]interface{}{ - "dbConfigMap": reconcilerName, + db1 := f.AddMockedUnstructuredPostgresDatabaseCR("db1") + { + runtimeStatus := map[string]interface{}{ + "dbConfigMap": "db1", + } + err := unstructured.SetNestedMap(db1.Object, runtimeStatus, "status") + require.NoError(t, err) + } + + db2 := f.AddMockedUnstructuredPostgresDatabaseCR("db2") + { + runtimeStatus := map[string]interface{}{ + "dbConfigMap": "db2", + } + err := unstructured.SetNestedMap(db2.Object, runtimeStatus, "status") + require.NoError(t, err) + } + + // create the ServiceBindingRequest + sbrSingleService := &v1alpha1.ServiceBindingRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps.openshift.io/v1alpha1", + Kind: "ServiceBindingRequest", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "single-sbr", + }, + Spec: v1alpha1.ServiceBindingRequestSpec{ + ApplicationSelector: v1alpha1.ApplicationSelector{ + MatchLabels: matchLabels, + Group: d.GetObjectKind().GroupVersionKind().Group, + Version: d.GetObjectKind().GroupVersionKind().Version, + Resource: "deployments", + ResourceRef: d.GetName(), + }, + BackingServiceSelectors: []v1alpha1.BackingServiceSelector{ + { + GroupVersionKind: db1.GetObjectKind().GroupVersionKind(), + ResourceRef: db1.GetName(), + }, + }, + }, + Status: v1alpha1.ServiceBindingRequestStatus{}, } - err := unstructured.SetNestedMap(cr.Object, runtimeStatus, "status") - require.NoError(t, err) + f.AddMockResource(sbrSingleService) // create the ServiceBindingRequest - sbr := &v1alpha1.ServiceBindingRequest{ + sbrMultipleServices := &v1alpha1.ServiceBindingRequest{ TypeMeta: metav1.TypeMeta{ APIVersion: "apps.openshift.io/v1alpha1", Kind: "ServiceBindingRequest", }, ObjectMeta: metav1.ObjectMeta{ - Name: reconcilerName, + Name: "multiple-sbr", }, Spec: v1alpha1.ServiceBindingRequestSpec{ ApplicationSelector: v1alpha1.ApplicationSelector{ @@ -144,44 +184,48 @@ func TestServiceBinder_Bind(t *testing.T) { Group: d.GetObjectKind().GroupVersionKind().Group, Version: d.GetObjectKind().GroupVersionKind().Version, Resource: "deployments", - ResourceRef: reconcilerName, + ResourceRef: d.GetName(), }, BackingServiceSelectors: []v1alpha1.BackingServiceSelector{ { - GroupVersionKind: cr.GetObjectKind().GroupVersionKind(), - ResourceRef: reconcilerName, + GroupVersionKind: db1.GetObjectKind().GroupVersionKind(), + ResourceRef: db1.GetName(), + }, + { + GroupVersionKind: db2.GetObjectKind().GroupVersionKind(), + ResourceRef: "db2", }, }, }, Status: v1alpha1.ServiceBindingRequestStatus{}, } - f.AddMockResource(sbr) + f.AddMockResource(sbrMultipleServices) logger := log.NewLog("service-binder") - t.Run("bind golden path", assertBind(args{ + t.Run("single bind golden path", assertBind(args{ options: &ServiceBinderOptions{ Logger: logger, DynClient: f.FakeDynClient(), DetectBindingResources: false, EnvVarPrefix: "", - SBR: sbr, + SBR: sbrSingleService, Client: f.FakeClient(), }, wantedActions: []wantedAction{ { resource: "servicebindingrequests", verb: "update", - name: reconcilerName, + name: sbrSingleService.GetName(), }, { resource: "secrets", verb: "update", - name: reconcilerName, + name: sbrSingleService.GetName(), }, { resource: "databases", verb: "update", - name: reconcilerName, + name: db1.GetName(), }, }, })) @@ -192,7 +236,7 @@ func TestServiceBinder_Bind(t *testing.T) { DynClient: f.FakeDynClient(), DetectBindingResources: true, EnvVarPrefix: "", - SBR: sbr, + SBR: sbrSingleService, Client: f.FakeClient(), }, })) @@ -209,4 +253,37 @@ func TestServiceBinder_Bind(t *testing.T) { }, wantBuildErr: InvalidOptionsErr, })) + + t.Run("multiple services bind golden path", assertBind(args{ + options: &ServiceBinderOptions{ + Logger: logger, + DynClient: f.FakeDynClient(), + DetectBindingResources: false, + EnvVarPrefix: "", + SBR: sbrMultipleServices, + Client: f.FakeClient(), + }, + wantedActions: []wantedAction{ + { + resource: "servicebindingrequests", + verb: "update", + name: sbrMultipleServices.GetName(), + }, + { + resource: "secrets", + verb: "update", + name: sbrMultipleServices.GetName(), + }, + { + resource: "databases", + verb: "update", + name: db1.GetName(), + }, + { + resource: "databases", + verb: "update", + name: db2.GetName(), + }, + }, + })) } From dcc69cdf16cdf86921534d7b6dea3113b3a18d37 Mon Sep 17 00:00:00 2001 From: Igor Sutton Lopes Date: Tue, 14 Jan 2020 14:05:43 +0100 Subject: [PATCH 27/29] Fixes go mod invalid pseudo-version error With the current version specified in go.mod, the following error happens: $ go mod vendor go: github.com/openshift/api@v3.9.1-0.20190424152011-77b8897ec79a+incompatible: invalid pseudo-version: preceding tag (v3.9.0) not found This patch replaces it with a valid pseudo-version encoding the same commit specified in the original. --- go.mod | 1 + go.sum | 2 ++ vendor/modules.txt | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 5eecce00bc..0c49128580 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( // Pinned to kubernetes-1.15.4 replace ( + github.com/openshift/api => github.com/openshift/api v0.0.0-20190424152011-77b8897ec79a k8s.io/api => k8s.io/api v0.0.0-20190918195907-bd6ac527cfd2 k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.0.0-20190918201827-3de75813f604 k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20190817020851-f2f3a405f61d diff --git a/go.sum b/go.sum index cf78eed484..2a92f95d91 100644 --- a/go.sum +++ b/go.sum @@ -439,6 +439,8 @@ github.com/opencontainers/image-spec v0.0.0-20170604055404-372ad780f634/go.mod h github.com/opencontainers/runc v0.0.0-20181113202123-f000fe11ece1/go.mod h1:qT5XzbpPznkRYVz/mWwUaVBUv2rmF59PVA73FjuZG0U= github.com/opencontainers/runtime-spec v1.0.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0= github.com/opencontainers/selinux v0.0.0-20170621221121-4a2974bf1ee9/go.mod h1:+BLncwf63G4dgOzykXAxcmnFlUaOlkDdmw/CqsW6pjs= +github.com/openshift/api v0.0.0-20190424152011-77b8897ec79a h1:zJauc4Mzrbn2C+G6cMwvvMCGWQZoyHaDlhoP6AjQDs8= +github.com/openshift/api v0.0.0-20190424152011-77b8897ec79a/go.mod h1:dh9o4Fs58gpFXGSYfnVxGR9PnV53I8TW84pQaJDdGiY= github.com/openshift/api v3.9.1-0.20190424152011-77b8897ec79a+incompatible h1:q2JBuObKafI7B4Eli6eLd+2T5JsU9ioWZ82zQwyjJPg= github.com/openshift/api v3.9.1-0.20190424152011-77b8897ec79a+incompatible/go.mod h1:dh9o4Fs58gpFXGSYfnVxGR9PnV53I8TW84pQaJDdGiY= github.com/openshift/client-go v0.0.0-20190401163519-84c2b942258a/go.mod h1:6rzn+JTr7+WYS2E1TExP4gByoABxMznR6y2SnUIkmxk= diff --git a/vendor/modules.txt b/vendor/modules.txt index c085540dc6..77052dc4b3 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -129,7 +129,7 @@ github.com/mitchellh/go-homedir github.com/modern-go/concurrent # github.com/modern-go/reflect2 v1.0.1 github.com/modern-go/reflect2 -# github.com/openshift/api v3.9.1-0.20190424152011-77b8897ec79a+incompatible +# github.com/openshift/api v3.9.1-0.20190424152011-77b8897ec79a+incompatible => github.com/openshift/api v0.0.0-20190424152011-77b8897ec79a github.com/openshift/api/apps/v1 github.com/openshift/api/route/v1 # github.com/operator-backing-service-samples/postgresql-operator v0.0.0-20191023140509-5c3697ed3069 From e75305c0d57b9f5d8274f28e72ea039586ce81b4 Mon Sep 17 00:00:00 2001 From: Igor Sutton Lopes Date: Tue, 14 Jan 2020 15:46:43 +0100 Subject: [PATCH 28/29] Extracts RelatedResources type to its own file --- .../servicebindingrequest/planner.go | 19 --------------- .../related_resources.go | 24 +++++++++++++++++++ 2 files changed, 24 insertions(+), 19 deletions(-) create mode 100644 pkg/controller/servicebindingrequest/related_resources.go diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go index 991038939a..ac2bb2b61a 100644 --- a/pkg/controller/servicebindingrequest/planner.go +++ b/pkg/controller/servicebindingrequest/planner.go @@ -3,7 +3,6 @@ package servicebindingrequest import ( "context" - olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -60,24 +59,6 @@ func (p *Planner) searchCRD(gvk schema.GroupVersionKind) (*unstructured.Unstruct return p.client.Resource(CRDGVR).Get(crdName, metav1.GetOptions{}) } -// RelatedResource represents a SBR related resource, composed by its CR and CRDDescription. -type RelatedResource struct { - CRDDescription *olmv1alpha1.CRDDescription - CR *unstructured.Unstructured -} - -// RelatedResources contains a collection of SBR related resources. -type RelatedResources []*RelatedResource - -// GetCRs returns a slice of unstructured CRs contained in the collection. -func (rr RelatedResources) GetCRs() []*unstructured.Unstructured { - var crs []*unstructured.Unstructured - for _, r := range rr { - crs = append(crs, r.CR) - } - return crs -} - // Plan by retrieving the necessary resources related to binding a service backend. func (p *Planner) Plan() (*Plan, error) { ns := p.sbr.GetNamespace() diff --git a/pkg/controller/servicebindingrequest/related_resources.go b/pkg/controller/servicebindingrequest/related_resources.go new file mode 100644 index 0000000000..3f0dfb7210 --- /dev/null +++ b/pkg/controller/servicebindingrequest/related_resources.go @@ -0,0 +1,24 @@ +package servicebindingrequest + +import ( + "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// RelatedResource represents a SBR related resource, composed by its CR and CRDDescription. +type RelatedResource struct { + CRDDescription *v1alpha1.CRDDescription + CR *unstructured.Unstructured +} + +// RelatedResources contains a collection of SBR related resources. +type RelatedResources []*RelatedResource + +// GetCRs returns a slice of unstructured CRs contained in the collection. +func (rr RelatedResources) GetCRs() []*unstructured.Unstructured { + var crs []*unstructured.Unstructured + for _, r := range rr { + crs = append(crs, r.CR) + } + return crs +} From bc35d128f0e4e0aab85a27cbc378454d88a947ed Mon Sep 17 00:00:00 2001 From: Igor Sutton Lopes Date: Tue, 14 Jan 2020 15:58:29 +0100 Subject: [PATCH 29/29] Extract methods to their own files This is in preparation to check-out those files in master before changing existing code. --- .../servicebindingrequest/binding.go | 13 +--- .../servicebindingrequest/plan_decoupled.go | 13 ++++ .../retrieve_decoupled.go | 61 +++++++++++++++++++ .../servicebindingrequest/retriever.go | 53 ---------------- 4 files changed, 77 insertions(+), 63 deletions(-) create mode 100644 pkg/controller/servicebindingrequest/plan_decoupled.go create mode 100644 pkg/controller/servicebindingrequest/retrieve_decoupled.go diff --git a/pkg/controller/servicebindingrequest/binding.go b/pkg/controller/servicebindingrequest/binding.go index 20819075ca..4a7a429543 100644 --- a/pkg/controller/servicebindingrequest/binding.go +++ b/pkg/controller/servicebindingrequest/binding.go @@ -9,7 +9,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "sigs.k8s.io/controller-runtime/pkg/client" @@ -282,7 +281,7 @@ func BuildServiceBinder(options *ServiceBinderOptions) (*ServiceBinder, error) { return nil, err } - rs := plan.RelatedResources.GetCRs() + rs := plan.GetCRs() // append all SBR related CRs objs = append(objs, rs...) @@ -291,20 +290,14 @@ func BuildServiceBinder(options *ServiceBinderOptions) (*ServiceBinder, error) { // read bindable data from the specified resources if options.DetectBindingResources { - err := retriever.ReadBindableResourcesData( - &plan.SBR, rs, []schema.GroupVersionResource{ - {Group: "", Version: "v1", Resource: "configmaps"}, - {Group: "", Version: "v1", Resource: "services"}, - {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, - }, - ) + err := retriever.ReadBindableResourcesData(&plan.SBR, rs) if err != nil { return nil, err } } // read bindable data from the CRDDescription found by the planner - for _, r := range plan.RelatedResources { + for _, r := range plan.GetRelatedResources() { err = retriever.ReadCRDDescriptionData(r.CR, r.CRDDescription) if err != nil { return nil, err diff --git a/pkg/controller/servicebindingrequest/plan_decoupled.go b/pkg/controller/servicebindingrequest/plan_decoupled.go new file mode 100644 index 0000000000..9252cfaa3d --- /dev/null +++ b/pkg/controller/servicebindingrequest/plan_decoupled.go @@ -0,0 +1,13 @@ +package servicebindingrequest + +import "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + +// GetCRs returns a slice of unstructured CRs contained in the internal related resources collection. +func (p *Plan) GetCRs() []*unstructured.Unstructured { + return p.RelatedResources.GetCRs() +} + +// GetRelatedResources returns the collection of related resources enumerated in the plan. +func (p *Plan) GetRelatedResources() RelatedResources { + return p.RelatedResources +} diff --git a/pkg/controller/servicebindingrequest/retrieve_decoupled.go b/pkg/controller/servicebindingrequest/retrieve_decoupled.go new file mode 100644 index 0000000000..ddf2c8d450 --- /dev/null +++ b/pkg/controller/servicebindingrequest/retrieve_decoupled.go @@ -0,0 +1,61 @@ +package servicebindingrequest + +import ( + "fmt" + + v1alpha12 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" +) + +// Retrieve returns the data read from related resources (see ReadBindableResourcesData and +// ReadCRDDescriptionData). +func (r *Retriever) Retrieve() (map[string][]byte, error) { + return r.data, nil +} + +// ReadBindableResourcesData reads all related resources of a given sbr +func (r *Retriever) ReadBindableResourcesData( + sbr *v1alpha1.ServiceBindingRequest, + crs []*unstructured.Unstructured, +) error { + r.logger.Info("Detecting extra resources for binding...") + for _, cr := range crs { + b := NewDetectBindableResources(sbr, cr, []schema.GroupVersionResource{ + {Group: "", Version: "v1", Resource: "configmaps"}, + {Group: "", Version: "v1", Resource: "services"}, + {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, + }, r.client) + + vals, err := b.GetBindableVariables() + if err != nil { + return err + } + for k, v := range vals { + r.store(cr, k, []byte(fmt.Sprintf("%v", v))) + } + } + + return nil +} + +// ReadCRDDescriptionData reads data related to given crdDescription +func (r *Retriever) ReadCRDDescriptionData(u *unstructured.Unstructured, crdDescription *v1alpha12.CRDDescription) error { + r.logger.Info("Looking for spec-descriptors in 'spec'...") + for _, specDescriptor := range crdDescription.SpecDescriptors { + if err := r.read(u, "spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { + return err + } + } + + r.logger.Info("Looking for status-descriptors in 'status'...") + for _, statusDescriptor := range crdDescription.StatusDescriptors { + if err := r.read(u, "status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go index b75164ba26..193c1cb597 100644 --- a/pkg/controller/servicebindingrequest/retriever.go +++ b/pkg/controller/servicebindingrequest/retriever.go @@ -5,13 +5,11 @@ import ( "fmt" "strings" - olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" - "github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1" "github.com/redhat-developer/service-binding-operator/pkg/log" ) @@ -258,57 +256,6 @@ func (r *Retriever) store(u *unstructured.Unstructured, key string, value []byte r.data[key] = value } -// Retrieve returns the data read from related resources (see ReadBindableResourcesData and -// ReadCRDDescriptionData). -func (r *Retriever) Retrieve() (map[string][]byte, error) { - return r.data, nil -} - -// ReadBindableResourcesData reads all related resources of a given sbr -func (r *Retriever) ReadBindableResourcesData( - sbr *v1alpha1.ServiceBindingRequest, - crs []*unstructured.Unstructured, - resources []schema.GroupVersionResource, -) error { - r.logger.Info("Detecting extra resources for binding...") - for _, cr := range crs { - b := NewDetectBindableResources(sbr, cr, []schema.GroupVersionResource{ - {Group: "", Version: "v1", Resource: "configmaps"}, - {Group: "", Version: "v1", Resource: "services"}, - {Group: "route.openshift.io", Version: "v1", Resource: "routes"}, - }, r.client) - - vals, err := b.GetBindableVariables() - if err != nil { - return err - } - for k, v := range vals { - r.store(cr, k, []byte(fmt.Sprintf("%v", v))) - } - } - - return nil -} - -// ReadCRDDescriptionData reads data related to given crdDescription -func (r *Retriever) ReadCRDDescriptionData(u *unstructured.Unstructured, crdDescription *olmv1alpha1.CRDDescription) error { - r.logger.Info("Looking for spec-descriptors in 'spec'...") - for _, specDescriptor := range crdDescription.SpecDescriptors { - if err := r.read(u, "spec", specDescriptor.Path, specDescriptor.XDescriptors); err != nil { - return err - } - } - - r.logger.Info("Looking for status-descriptors in 'status'...") - for _, statusDescriptor := range crdDescription.StatusDescriptors { - if err := r.read(u, "status", statusDescriptor.Path, statusDescriptor.XDescriptors); err != nil { - return err - } - } - - return nil -} - // NewRetriever instantiate a new retriever instance. func NewRetriever(client dynamic.Interface, plan *Plan, bindingPrefix string) *Retriever { return &Retriever{