Feedback: allowing customEnvVar to be defined per service in backingServiceSelectors

[navidsh]

Current ServiceBindingRequest CRD doesn't allow users to define customEnvVar per service in the backingServiceSelectors list. This limits users to define multiple backing services and use customEnvVar to extract info from the backing services with the same GVK.

SBR:

  backingServiceSelectors:
    - group: serving.knative.dev
      kind: Service
      resourceRef: demo-1
      version: v1beta1
    - group: serving.knative.dev
      kind: Service
      resourceRef: demo-2
      version: v1beta1
  customEnvVar:
    - name: KSVC_URL
       value: {{ .status.url }}

Backing service:

apiVersion: serving.knative.dev/v1beta1
kind: Service
metadata:
  name: demo-1
  namespace: service-binding-demo
...
status:
  url: 'http://demo-1.service-binding-demo.apps.talking.os.fyre.ibm.com'

Users can create separate SBR for each backing service to get around this limitation.

[sbose78]

@navidsh We are going to support the following in the customEnvVar to let users compose environment variables from one or more backing services.

customEnvVar:
    - name: KSVC_URL
       value: {{ Service.demo2.status.url }}

In other words,

{{ .<kind>.<resourceRef>.status.<attribute> }}

[sbose78]

Something like..

 backingServiceSelectors:
    - group: postgres.dev
      kind: Service
      resourceRef: user-db
      version: v1beta1
    - group: postgres.dev
      kind: Credentials
      resourceRef: admin-creds
      version: v1beta1
  customEnvVar:
    - name: CONNECTION_URL
       value: {{  Credentials.admin-creds.spec.username }} / {{ Service.user-db.status.url }}

[navidsh]

Thanks @sbose78!

Just wanted to throw it out there, how about moving customEnvVar into each of the backingServiceSelectors items e.g.:

 backingServiceSelectors:
    - group: postgres.dev
      kind: Service
      resourceRef: user-db
      version: v1beta1
    - group: postgres.dev
      kind: Credentials
      resourceRef: admin-creds
      version: v1beta1
      customEnvVar:
        - name: CONNECTION_URL
          value: {{ .spec.username }} / {{ Service.user-db.status.url }}

I agree that it looks clean to have a single customEnvVar in the whole CRD to list all the custom env variables. But then pointing to a backing service in the value might not be very obvious to users.

[sbose78]

thanks!

@pedjak do you want to outline your suggestion here?

[Avni-Sharma]

I agree with @navidsh and feel that custEnvVar is kind of implementation(and an optional) detail and it would be better if it sits per backing service basis. So that the CR has the important aspects i.e the Applications and Services .

[arthurdm]

In this topic, if we move customEnvVar to be under each of the services, I would suggest we rename the key to dataMapping - this allows it to remain neutral in terms of being injected as a set of environment variables versus mounted files. I am actually using that terminology in a draft of a future PR I will make to the service binding spec (see the 3rd bullet under the Minimum set in here).

Example:

 services:
    - group: postgres.dev
      kind: Service
      resourceRef: user-db
      version: v1beta1
      dataMapping:
        - name: postgres_name
          value: {spec.dbName}
        - name: postgres_host
          value: {status.dbConnectionIP}
        - name: postgres_port
          value: {status.dbConnectionPort}
        - name: postgres_protocol
          value: jdbc:postgresql

[sbose78]

I'm good with the renaming.

Avni, Navid,
How would we compose variables from multiple backing services if we move this under individual backing services?

[pedjak]

How about that we replace using .. pairs with a logical name (like var name) that can be associated to a resource within a ServiceBindingRequest instance. At the same we can collapse into resourceRef field the other fields: group, version and plural/singular CRD name. For example:

backingServiceSelectors:
    - resourceRef: postgres.dev/v1beta1/services/user-db
      name: myUserDb

and then we can craft expressions like

{{ .myUserDb.spec.<attribute> }}

name field could be optional, and if not there user-db could be used as long as is the only resource with that name within the given ServiceBindingRequest instance:

{{ .user-db.spec.<attribute> }}

In case that we have only one resourceRef, we can still use the old syntax {{ .spec.attribute }}, because it is clear what resource we speak about.

[sbose78]

resourceRef: postgres.dev/v1beta1/services/user-db

We were doing something similar but realized it was a non-standard kubernetes API and needed explicit parsing/deserialization compared to using https://godoc.org/k8s.io/apimachinery/pkg/apis/meta/v1#GroupVersionKind

I really like the second part of your proposal @pedjak , how about designing the API like this:

backingServiceSelectors:
    - group: postgres.dev
      kind: Service
      resourceRef: user-db
      version: v1beta1
      id: X
    - group: postgres.dev
      kind: Credentials
      resourceRef: admin-creds
      version: v1beta1
      id: Y
  customEnvVar:
    - name: CONNECTION_URL
       value: {{  X.status.username }} / {{ Y.status.url }}

[pedjak]

I really like the second part of your proposal @pedjak , how about designing the API like this:

@sbose78 fine, but in that case, I would rename resourceRef to name, because the tuple (group, kind, version, name) is actually resourceRef. And in case if we really want to use the existing GroupVersionKind type, we could design backingServiceSelectors as follows:

backingServiceSelectors:
    - resourceRef:
         group: postgres.dev
         kind: Service
         name: user-db
         version: v1beta1
      id: X

And since we are here: could we drop custom prefix from customEnvVar and call it envVars? or custom brings some additional semantic?

[navidsh]

I like adding the id approach to allow composing custom variables from multiple backing services 👍

[sbose78]

We'll solve this as part of #387

[arthurdm]

+1 to the addition of id. We're utilizing this concept in the service binding spec.

Did we also agree on renaming resourceRef to name and renaming customEnvVar to dataMapping?

[sbose78]

but in that case, I would rename resourceRef to name, because the tuple (group, kind, version, name) is actually resourceRef.

I really like this, but doing this would break the UI and therefore it would take us longer to put the change in.

[yharish991]

why don't we have customEnvVar per backing service basis? something like this

backingServiceSelectors:
    - group: postgres.dev
      kind: Service
      resourceRef: user-db
      version: v1beta1
      id: X
    - group: postgres.dev
      kind: Credentials
      resourceRef: admin-creds
      version: v1beta1
      id: Y
      customEnvVar:
         - name: CONNECTION_URL
         value: {{  X.status.username }} / {{ Y.status.url }}

It is quite common in the SQL world to refer to multiple tables all over the query, so was thinking why not here, the developers are already used to this kind of thinking. Also it solves the problem of identifying binding info. How do we identify which service the CONNECTION_URL belongs when there are multiple backing services of different types(from the application perspective).

More context in here servicebinding/spec#41 (review)

[pedjak]

Implemented - check https://github.com/redhat-developer/service-binding-operator/blob/master/docs/application-author.md#custom-binding-variables for usage.