Support creation of managed secret without associating it to a workload #350

DhritiShikhar · 2020-02-26T12:33:41Z

Task: https://issues.redhat.com/browse/APPSVC-9

Description
Add support for "generic binding".
"In the service catalog UX this meant I just want the information dumped into a Secret, and then I'll choose what to do with it after that. This is good for referencing the Secret from other things like Jobs, or a Tekton pipeline, where your controller may not be able to directly modify a pod spec.

The request for this is coming up in discussions about transitioning some IBM brokers off of service catalog to operators instead. I think the use case is valid so we should consider it."

Acceptance Criteria

If the ApplicationSelectors is skipped, only the binding secret should be created with the name of the secret being dumped in the status for arbitrary consumption.
If the CR is updated with the ApplicationSelectors section, then the controller should inject the previously created secret into the deployment/deploymentconfig.
Irrespective of (2), If the credentials change on the backing service side, the binding secret should be updated as well

DhritiShikhar · 2020-02-26T12:59:14Z

/assign @isutton

codecov · 2020-02-26T13:58:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@58ea511). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #350   +/-   ##
=========================================
  Coverage          ?   64.74%           
=========================================
  Files             ?       21           
  Lines             ?     1387           
  Branches          ?        0           
=========================================
  Hits              ?      898           
  Misses            ?      364           
  Partials          ?      125

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58ea511...78118e8. Read the comment docs.

sbose78 · 2020-02-26T14:11:43Z

I don't see code changes, only tests?

DhritiShikhar · 2020-02-26T14:19:12Z

@sbose78 All the 3 acceptance criteria is already supported. I wrote test to confirm the 3 acceptance criteria in the task:

If the ApplicationSelectors is skipped, only the binding secret should be created with the name of the secret being dumped in the status for arbitrary consumption.
If the CR is updated with the ApplicationSelectors section, then the controller should inject the previously created secret into the deployment/deploymentconfig.
Irrespective of (2), If the credentials change on the backing service side, the binding secret should be updated as well.

sbose78 · 2020-02-26T15:24:11Z

Interesting, Thank you!

sbose78 · 2020-02-26T15:52:52Z

The API validations don't let it go through..

sbose78 · 2020-02-26T16:42:12Z

.. but you would then get this

2020-02-26T11:29:56.327-0500    DEBUG   secret  Secret already exists, updating contents instead...     {"Namespace": "sbose", "Name": "binding-request"}
E0226 11:29:56.372848     560 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 914 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic(0x2127980, 0x30586a0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0xa3
k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x82
panic(0x2127980, 0x30586a0)
        /usr/local/Cellar/go/1.13.8/libexec/src/runtime/panic.go:679 +0x1b2
github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest.(*Binder).search(0xc00027bec0, 0x2, 0xc0004559a0, 0xf)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest/binder.go:53 +0x7c
github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest.(*Binder).Bind(0xc00027bec0, 0xc00088da28, 0x2, 0x2, 0xc0004559a0, 0xf)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest/binder.go:539 +0x2f
github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest.(*ServiceBinder).Bind(0xc0001b34a0, 0x232b02e, 0x30, 0x0, 0x0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest/binding.go:232 +0x179
github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest.(*Reconciler).bind(0xc0003b2ba0, 0xc000780100, 0xc0001b34a0, 0xc00034a1b8, 0x0, 0x0, 0x0, 0x0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest/reconciler.go:93 +0xde
github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest.(*Reconciler).Reconcile(0xc0003b2ba0, 0xc000b02fd9, 0x5, 0xc000b02fc0, 0xf, 0xc00088dcd8, 0xc0007485a0, 0xc000748518, 0x25002e0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/pkg/controller/servicebindingrequest/reconciler.go:176 +0x5c2
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc000366cc0, 0x217efe0, 0xc00038c380, 0x0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:256 +0x162
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc000366cc0, 0xc00012a700)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:232 +0xcb
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker(0xc000366cc0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:211 +0x2b
k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1(0xc00089a8d0)
        /Users/sbose/appbinding/src/github.com/redhat-developer/service-binding-operator/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:152 +0x5e
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc00089a8d0, 0x3b9aca00, 0x0, 0xc000ab0501, 0xc0000be240)

sbose78 · 2020-02-26T16:56:48Z

I tried making a PR to your branch but the history was a bit messed up ( my bad probably ), so created one against the main repo:
#351 . I will not be merging #351, we'll merge your PR ( current one ).

I've tested #351, it works, but doesn't provide any feedback in the status block.

Could you please take in the changes, and then..
make improvements if needed,
ensure status is updated
add more tests based on the new changes

Here's the only relevant commit there.
ef6f306

isutton · 2020-02-27T13:50:34Z

pkg/apis/apps/v1alpha1/zz_generated.openapi.go

-		"./pkg/apis/apps/v1alpha1.ApplicationSelector":         schema_pkg_apis_apps_v1alpha1_ApplicationSelector(ref),
-		"./pkg/apis/apps/v1alpha1.BackingServiceSelector":      schema_pkg_apis_apps_v1alpha1_BackingServiceSelector(ref),
-		"./pkg/apis/apps/v1alpha1.ServiceBindingRequest":       schema_pkg_apis_apps_v1alpha1_ServiceBindingRequest(ref),
-		"./pkg/apis/apps/v1alpha1.ServiceBindingRequestSpec":   schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestSpec(ref),
-		"./pkg/apis/apps/v1alpha1.ServiceBindingRequestStatus": schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestStatus(ref),
+		"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.ApplicationSelector":         schema_pkg_apis_apps_v1alpha1_ApplicationSelector(ref),
+		"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.BackingServiceSelector":      schema_pkg_apis_apps_v1alpha1_BackingServiceSelector(ref),
+		"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.ServiceBindingRequest":       schema_pkg_apis_apps_v1alpha1_ServiceBindingRequest(ref),
+		"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.ServiceBindingRequestSpec":   schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestSpec(ref),
+		"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1.ServiceBindingRequestStatus": schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestStatus(ref),


This is not the first time I've seen this; why generated files are being generated differently between developers?

Maybe because of operator-sdk versions? I'm using 0.5.2

Mine is different:

➜ service-binding-operator git:(generic) ✗ operator-sdk version operator-sdk version: "v0.15.0-37-g0eac8a11", commit: "0eac8a1169dfc0a8918b9e41ada6278307a0852b", go version: "go1.13.6 linux/amd64"

You definitely got to update your operator-sdk CLI
:-)

I am on 0.15.2 and even for me these changes are happening

isutton · 2020-02-28T15:07:01Z

I tried making a PR to your branch but the history was a bit messed up ( my bad probably ), so created one against the main repo:
#351 . I will not be merging #351, we'll merge your PR ( current one ).

I've tested #351, it works, but doesn't provide any feedback in the status block.
* Could you please take in the changes, and then..

* make improvements if needed,

* ensure status is updated

* add more tests based on the new changes
Here's the only relevant commit there.
ef6f306

Shouldn't we leave those changes for another PR, since the implemented tests already seem to validate the initial assumption that it currently works?

sbose78 · 2020-02-28T21:31:13Z

Shouldn't we leave those changes for another PR, since the implemented tests already seem to validate the initial assumption that it currently works?

Sorry, which changes are you saying we should skip in this PR ?

sbose78 · 2020-03-04T20:28:55Z

How's this going? @DhritiShikhar

isutton · 2020-03-10T10:11:26Z

Shouldn't we leave those changes for another PR, since the implemented tests already seem to validate the initial assumption that it currently works?

Sorry, which changes are you saying we should skip in this PR ?

Related to the lack of feedback in the status block.

sbose78 · 2020-03-10T12:32:11Z

But the status is empty.

DhritiShikhar · 2020-03-10T13:01:42Z

@sbose78 Hmm.. but I added check for status in my test. It's not empty there -> 572adc7

Both status.BindingStatus and status.Conditions are non-empty with desirable result.

DhritiShikhar · 2020-03-11T11:39:41Z

@Avni-Sharma addressed. Please review again f2c599a

Avni-Sharma · 2020-03-12T07:47:05Z

pkg/controller/servicebindingrequest/reconciler_test.go

+
+	require.Equal(t, "BindingSuccess", sbrOutput3.Status.BindingStatus)
+	require.Equal(t, corev1.ConditionTrue, sbrOutput3.Status.Conditions[0].Status)
+	require.Equal(t, reconcilerName, sbrOutput3.Status.Secret)


Can we have sbrOutput3.Status.BindingData.Name . Now that the bindingData can be either in a Secret or a ConfigMap according to #366 . Refer https://github.com/redhat-developer/service-binding-operator/pull/366/files#diff-14a37dadc390fc3ec23459efc088de8eR164-R180 cc @sbose78

but #366 is not merged yet

we can track this and send a new PR :)

DhritiShikhar · 2020-03-12T07:58:19Z

/retest

Avni-Sharma · 2020-03-12T08:57:08Z

pkg/controller/servicebindingrequest/reconciler_test.go

+
+func TestReconcilerGenericBinding(t *testing.T) {
+	ctx := context.TODO()
+	backingServiceResourceRef := "test-using-volumes"


We are not using volumes for this test case so we should rename "test-using-volumes"

fixed 78118e8

Avni-Sharma · 2020-03-12T12:20:39Z

/lgtm

DhritiShikhar · 2020-03-12T15:19:11Z

/approve

openshift-ci-robot · 2020-03-12T15:19:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DhritiShikhar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DhritiShikhar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

DhritiShikhar · 2020-03-12T15:20:40Z

@sbose78 Can you merge this PR (if no more comments) ?

DhritiShikhar · 2020-03-12T16:58:43Z

/retest

sbose78 · 2020-03-12T21:06:35Z

Could you share an example we've tested this with ? I'm getting the following:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  creationTimestamp: '2020-03-12T21:05:44Z'
  generation: 1
  name: node-todo-git
  namespace: sbose
  resourceVersion: '221728'
  selfLink: >-
    /apis/apps.openshift.io/v1alpha1/namespaces/sbosse/servicebindingrequests/node-todo-git
  uid: 3413f95b-7cd6-44b7-9a62-daaec834be18
spec:
  backingServiceSelectors:
    - group: etcd.database.coreos.com
      kind: EtcdCluster
      resourceRef: example
      version: v1beta2
  detectBindingResources: true
status:
  bindingStatus: BindingFail
  conditions:
    - lastHeartbeatTime: '2020-03-12T21:05:59Z'
      lastTransitionTime: '2020-03-12T21:05:45Z'
      message: application ResourceRef or MatchLabel not found
      reason: BindingFail
      status: 'False'
      type: Ready
  secret: node-todo-git

sbose78 · 2020-03-12T21:07:24Z

The secret does get created though.

sbose78 · 2020-03-12T21:10:39Z

@DhritiShikhar Please use the issue template to specify "How to test". We should not be replacing the template with the content of a linked issue.

DhritiShikhar added 2 commits February 26, 2020 18:01

Adds test for Generic Binding

619810c

Merge remote-tracking branch 'origin/master' into generic

6495e55

openshift-ci-robot requested review from otaviof and sbose78 February 26, 2020 12:33

openshift-ci-robot added the size/M label Feb 26, 2020

update Binding Status

45abadc

openshift-ci-robot assigned isutton Feb 26, 2020

remove application selector from required

0d20483

makes application selector optional

415b1a8

openshift-ci-robot added size/L and removed size/M labels Feb 27, 2020

isutton reviewed Feb 27, 2020

View reviewed changes

sbose78 changed the title ~~Test for generic binding~~ Support creation of managed secret without associating it to a workload Feb 27, 2020

sbose78 mentioned this pull request Mar 2, 2020

Is it possible to opt out from injecting environment variables? #296

Closed

openshift-ci-robot added the needs-rebase label Mar 7, 2020

DhritiShikhar added 2 commits March 10, 2020 18:20

Checks status on reconcilation

572adc7

Merge remote-tracking branch 'origin/master' into generic

88203f1

openshift-ci-robot removed the needs-rebase label Mar 10, 2020

uses corev1.ConditionTrue and corev1.ConditionFalsee

f2c599a

remove CSV with volume mount

1eceb8e

Avni-Sharma reviewed Mar 12, 2020

View reviewed changes

change backingServiceResourceRef name

78118e8

openshift-ci-robot assigned Avni-Sharma Mar 12, 2020

openshift-ci-robot added the lgtm label Mar 12, 2020

openshift-ci-robot added the approved label Mar 12, 2020

openshift-merge-robot merged commit c0f53c1 into redhat-developer:master Mar 12, 2020

djones6 mentioned this pull request Sep 9, 2020

Update ServiceBindingRequests to work with latest Operator ibm-icpa-coffeeshop/gitops-dev#2

Merged

DhritiShikhar mentioned this pull request Sep 10, 2020

Create an overall Ready to indicate whether the SBR reconcile is completed successfully #665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support creation of managed secret without associating it to a workload #350

Support creation of managed secret without associating it to a workload #350

DhritiShikhar commented Feb 26, 2020 •

edited

Loading

DhritiShikhar commented Feb 26, 2020

codecov bot commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020

DhritiShikhar commented Feb 26, 2020

sbose78 commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020

sbose78 commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020 •

edited

Loading

isutton Feb 27, 2020

sbose78 Feb 27, 2020

DhritiShikhar Feb 28, 2020

sbose78 Mar 7, 2020 •

edited

Loading

Avni-Sharma Mar 12, 2020

isutton commented Feb 28, 2020

sbose78 commented Feb 28, 2020

sbose78 commented Mar 4, 2020

isutton commented Mar 10, 2020

sbose78 commented Mar 10, 2020

DhritiShikhar commented Mar 10, 2020

DhritiShikhar commented Mar 11, 2020

Avni-Sharma Mar 12, 2020

DhritiShikhar Mar 12, 2020

Avni-Sharma Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

Avni-Sharma Mar 12, 2020

DhritiShikhar Mar 12, 2020

Avni-Sharma commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

sbose78 commented Mar 12, 2020

sbose78 commented Mar 12, 2020

sbose78 commented Mar 12, 2020

Support creation of managed secret without associating it to a workload #350

Support creation of managed secret without associating it to a workload #350

Conversation

DhritiShikhar commented Feb 26, 2020 • edited Loading

DhritiShikhar commented Feb 26, 2020

codecov bot commented Feb 26, 2020 • edited Loading

Codecov Report

sbose78 commented Feb 26, 2020

DhritiShikhar commented Feb 26, 2020

sbose78 commented Feb 26, 2020 • edited Loading

sbose78 commented Feb 26, 2020

sbose78 commented Feb 26, 2020 • edited Loading

sbose78 commented Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isutton commented Feb 28, 2020

sbose78 commented Feb 28, 2020

sbose78 commented Mar 4, 2020

isutton commented Mar 10, 2020

sbose78 commented Mar 10, 2020

DhritiShikhar commented Mar 10, 2020

DhritiShikhar commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhritiShikhar commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avni-Sharma commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

DhritiShikhar commented Mar 12, 2020

sbose78 commented Mar 12, 2020

sbose78 commented Mar 12, 2020

sbose78 commented Mar 12, 2020

DhritiShikhar commented Feb 26, 2020 •

edited

Loading

codecov bot commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020 •

edited

Loading

sbose78 commented Feb 26, 2020 •

edited

Loading

sbose78 Mar 7, 2020 •

edited

Loading