Support link command for devfile

[elsony]

/kind user-story

User Story

As a user, I want to use the link command to link a service to a given component

Acceptance Criteria

• the link command should behave the same as the existing link s2i style command
• [ ] check and disallow self linking of components covered by self linking of the component should not be there using odo link #1052

Links

• Related issue:

/kind user-story
/area devfile

[kadel]

odo link command currently doesn't work as we want to. For s2i style components, it creates a link, but the link information is not recorded in config.yaml. This means that the user needs to create the service again each time he pushes the new component or after the component relation. This is not user-friendly as user needs to create it each time, and he/she needs to know the type of services the component requires and all service parameters.

There is also this issue that is related #2534

We might need to first figure out how to store the service information.
Storing it in devfile.yaml vs storing it in env.yaml.

Storing in devfile.yaml

Benefits:

• The service is always created with the component (odo push)
• It is possible to just do git clone ... && odo push and have everything running.

Challenges:

• devfile.yaml includes service, but I want to use service of the same type that I have already running

Storing in env.yaml

Benefits:

• I can easily delete and recreate service without passing all parameters again

Challenges:

• Can't do just simple git clone ... && odo push and have everything running (service will be missing)
• Parameters of the service are not recorded in the git repository, so I need to remember how to create the service

Not storing it at all

Benefits:

• simple to do

Challenges:

• Can't do just simple git clone ... && odo push and have everything running (service will be missing)
• Parameters of the service are not recorded in the git repository, so I need to remember how to create the service

[kadel]

/triage needs-information

[girishramnani]

the discussed workflow was

• to check if service binding operator is present on the cluster, if so use the ServiceBindingRequest as that allows the dev-console visualisation capabilities of that link.
• if the operator is not present then we use the logic present in the operator ( I discussed with service binding team and they mentioned that linking can happen without the operator too ).
• but preference has to be given to operator and custom logic needs to only be used as a fall back.

[dharmit]

I tried to link a nodejs devfile component with the etcd cluster by following the steps mentioned in https://github.com/redhat-developer/service-binding-operator/blob/master/examples/nodejs_etcd_operator/README.md. But it's not working yet.

So far, I think it's because service binding operator expects us to set name value in the metadata.labels for the Deployment created for a component. But I'm not entirely sure yet.

Will be looking at this more and keep this issue updated.

[kadel]

• if the operator is not present then we use the logic present in the operator ( I discussed with service binding team and they mentioned that linking can happen without the operator too )

+1, but let's break this into two stages. We should start with the implementation that uses ServiceBindingOperator and than add the fall back after we have this.

[dharmit]

• if the operator is not present then we use the logic present in the operator ( I discussed with service binding team and they mentioned that linking can happen without the operator too )

+1, but let's break this into two stages. We should start with the implementation that uses ServiceBindingOperator and than add the fall back after we have this.

+1. Even with SBO, it's not really apparent so far. The example I mentioned in #2920 (comment) is still not working. I'll open an issue on the project repo to get help from their experts.

[dharmit]

Had a call with a member from the SBO team and we tried to create a binding request. But it failed. Tracking it in redhat-developer/service-binding-operator#467.

[dharmit]

My research so far has been based on the SBO. I've not tried linking the component and service without the operator.

The yaml file that I used to create a binding between a component and etcd cluster is:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
 name: binding-request
 namespace: sbo
spec:
 applicationSelector:
   group: apps
   version: v1
   resource: deployments
   resourceref: nodejs-rest-http-crud
 backingServiceSelector:
   group: etcd.database.coreos.com
   kind: EtcdCluster
   version: v1beta2
   resourceRef: example
 mountPathPrefix: ""
 customEnvVar: []
 detectBindingResources: true

The code/API to do this is under https://github.com/redhat-developer/service-binding-operator/blob/master/pkg/controller/servicebindingrequest/servicebinder.go.

[dharmit]

There was a tiny error in the yaml because of which the linking didn't work. The linking wasn't working because I had used resourceref: nodejs-rest-http-crud instead of resourceRef: nodejs-rest-http-crud. That's resourceref instead of resourceRef.

I'll be adding https://github.com/redhat-developer/service-binding-operator/ as a dependency in odo to be able to use it to bind the components & services.

[dharmit]

Scope of this issue based on my research and understanding so far:

1. Add dependency on Service Binding Operator (SBO)
   a. Fix things that might break during such times

2. Understand how to use SBO programatically

3. Decide the command to be used to link a component with service. This is required because there could be multiple services from different backing CRs that have the same name:

   $ odo service list
   NAME        TYPE            AGE
   example     Cockroachdb     3s
   example     EtcdCluster     3m22s

   So doing odo link example like we did to link Service Catalog based services won't suffice.

   Should we do odo link <service-type>/<service-name> or something else? This would translate to doing odo link Cockroachdb/example from devfile component's directory for above output.

4. Add support to link devfile component with operator backed service

5. Tests

6. Docs

Above scope is based on the assumption that we'll be using SBO to link. Linking without SBO should be a separate issue/PR, IMO. It also doesn't account for what's discussed in #2920 (comment) because for that we first need to agree upon:

1. storing the info in either in env.yaml or devfile.yaml file, and
2. what needs to be stored

Above scope also doesn't include linking s2i components with the service. It should be a separate issue/PR, IMO.

[dharmit]

/area linking
/area service-operators

[dharmit]

I added dependency on the Service Binding Operator but the binary's failing to build with below error:

$ make
go build -ldflags="-w -X github.com/openshift/odo/pkg/version.GITCOMMIT=72190cfe5" cmd/odo/odo.go
# github.com/openshift/odo/pkg/lclient
pkg/lclient/fakeclient.go:65:3: cannot use container.HostConfig literal (type container.HostConfig) as type struct { NetworkMode string "json:\",omitempty\"" } in field value
pkg/lclient/fakeclient.go:84:3: cannot use container.HostConfig literal (type container.HostConfig) as type struct { NetworkMode string "json:\",omitempty\"" } in field value
pkg/lclient/fakeclient.go:148:6: cannot use &containerElement.HostConfig (type *struct { NetworkMode string "json:\",omitempty\"" }) as type *container.HostConfig in field value
make: *** [Makefile:53: bin] Error 2

Apparently the definition of Container has changed from:

// Container contains response of Engine API:
// GET "/containers/json"
type Container struct {
    ID              string `json:"Id"`
    Names           []string
    Image           string
    ImageID         string
    Command         string
    Created         int64
    Ports           []Port
    SizeRw          int64 `json:",omitempty"`
    SizeRootFs      int64 `json:",omitempty"`
    Labels          map[string]string
    State           string
    Status          string
    HostConfig      container.HostConfig
    NetworkSettings *SummaryNetworkSettings
    Mounts          []MountPoint
}

to:

// Container contains response of Engine API:
// GET "/containers/json"
type Container struct {
    ID         string `json:"Id"`
    Names      []string
    Image      string
    ImageID    string
    Command    string
    Created    int64
    Ports      []Port
    SizeRw     int64 `json:",omitempty"`
    SizeRootFs int64 `json:",omitempty"`
    Labels     map[string]string
    State      string
    Status     string
    HostConfig struct {
        NetworkMode string `json:",omitempty"`
    }   
    NetworkSettings *SummaryNetworkSettings
    Mounts          []MountPoint
}

I need help to move ahead on this. I'm kind of clueless as to how to fix this. How can I replace a struct with just a string?

To add the dependency I did:

$ glide get github.com/redhat-developer/service-binding-operator/
$ glide update -v

I've pushed the resulting repo to https://github.com/dharmit/odo/tree/fix-2920.

ping @yangcao77 (since I see that you worked on this in the past)

UPDATE: I've deleted the branch mentioned above in favour of just copying the structs to odo repo instead of adding dependency on SBO.

[elsony]

We should take the current purpose of the service binding specification into account when we design this feature: https://github.com/application-stacks/service-binding-specification/#application-projection

[dharmit]

@elsony the way we've done it right now is:

1. User will provide a command to link a component to an operator backed service:

   # From a component directory execute
   $ odo link EtcdCluster/example

2. odo will create a ServiceBindingRequest in memory and populate it with required information. Link has not been created yet.
3. As a final step, we create a "service" from the CR ServiceBindingRequest which is provided by the Operator "Service Binding Operator". The result of this service is a link (or a ServiceBindingRequest) which can be listed with odo service list or oc get csv.

To be able to execute this final step of link (or "service") creation, we create a YAML from the struct we're working with in step 2 and use the dynamic client to spin up the service.

So we're basically just offloading the task of mounting/injecting relevant information, from an Operator backed service into the component, to the Service Binding Operator.

However, if we intend to add some flags to link command, it would require us some discussion because that would deviate from how odo link works right now when linking a component with Service Catalog based component. Even in its current form, odo link deviates a bit because in case of Service Catalog, we would do something like odo link my-postgres where my-postgres is the name of the service. But in case of Operator backed service, there could be multiple services with the same name so we have to provide CR name as well: odo link EtcdCluster/example or odo link Cockroachdb/example.

[kadel]

3. As a final step, we create a "service" from the CR ServiceBindingRequest which is provided by the Operator "Service Binding Operator". The result of this service is a link (or a ServiceBindingRequest) which can be listed with odo service list or oc get csv.

Maybe I'm missing something. But odo service list should mainly list services. It can have additional information listing links for existing services, but the main thing that it should ist is services.

However, if we intend to add some flags to link command, it would require us some discussion because that would deviate from how odo link works right now when linking a component with Service Catalog based component. Even in its current form, odo link deviates a bit because in case of Service Catalog, we would do something like odo link my-postgres where my-postgres is the name of the service. But in case of Operator backed service, there could be multiple services with the same name so we have to provide CR name as well: odo link EtcdCluster/example or odo link Cockroachdb/example.

using odo link <CRD>/<name> is ok as long as we use the same system for listing services (odo service list)

[dharmit]

Maybe I'm missing something. But odo service list should mainly list services. It can have additional information listing links for existing services, but the main thing that it should ist is services.

We are creating a link using the "Service Binding Operator". In a way we're creating a service from an Operator. Hence, it shows up under odo service list. Indeed, the odo service list command lists the services only. But in this case, the link is a service of type "Service Binding Request" provided by the Operator "Service Binding Operator". Does that help clear the confusion?

using odo link <CRD>/<name> is ok as long as we use the same system for listing services (odo service list)

At the moment, odo service list simply lists the services in the current project/namespace. An output like below:

$ odo service list
NAME        TYPE            AGE
example     Cockroachdb     3s
example     EtcdCluster     3m22s

Can you elaborate on what you're expecting from odo service list when you say "ok as long as we use the same system for listing services"? I'm asking since I don't understand.

[kadel]

We are creating a link using the "Service Binding Operator". In a way we're creating a service from an Operator. Hence, it shows up under odo service list. Indeed, the odo service list command lists the services only. But in this case, the link is a service of type "Service Binding Request" provided by the Operator "Service Binding Operator". Does that help clear the confusion?

It does. But isn't this going to confuse odo users? We might try hide ServiceBindingRequests from the odo service list output.

Can you elaborate on what you're expecting from odo service list when you say "ok as long as we use the same system for listing services"? I'm asking since I don't understand.

It depends on how you define odo link command.

If help is going to say odo link <serviceName> than the odo service list needs to contain correct NAME field

$ odo service list
NAME                   AGE
example/Cockroachdb     3s
example/EtcdCluster     3m22s

If help is going to say odo link <NAME>/<TYPE> than the odo service list can have output as in your example

$ odo service list
NAME        TYPE            AGE
example     Cockroachdb     3s
example     EtcdCluster     3m22s

Also keep in mind that odo link still needs to be able to link to ServiceCatalog services and to other components. There is separate flag for linking to component so that might not be a problem.

[dharmit]

It does. But isn't this going to confuse odo users? We might try hide ServiceBindingRequests from the odo service list output.

Yes, I agree. It could confuse. The most epic situation would be when someone tries to link a component to an existing SBR. 😂

Tangential question/discussion which we could cover in a dedicated issue:

• Currently, for Service Catalog based services, we do not list the links, AFAIK. How do we want the behaviour to be in case of Operator backed services?
• There's also Unable to delete a service binding request if underlying application and/or service are deleted beforehand service-binding-operator#535 which I have opened based on my observation. It might result in dangling SBRs. Deleting a component odo delete and service odo service delete EtcdCluster/example (yet to be implemented), does not delete the SBR that was used to link the two.

If help is going to say odo link <serviceName> than the odo service list needs to contain correct NAME field

$ odo service list
NAME                    AGE
example/Cockroachdb     3s
example/EtcdCluster     3m22s

This might be a better option since we want components to link to either Operator backed service or Service Catalog backed service. But how about below instead:

$ odo service list
NAME                          AGE
Cockroachdb/example           3s
EtcdCluster/example           3m22s
EtcdCluster/myawesomeetcd     3s

It still follows the <NAME>/<TYPE> convention and makes more sense if we had more than one services deployed from a particular CR.

If help is going to say odo link <NAME>/<TYPE> than the odo service list can have output as in your example

$ odo service list
NAME        TYPE            AGE
example     Cockroachdb     3s
example     EtcdCluster     3m22s

But this is how we're listing things right now.

[dharmit]

Current status of this issue

odo link can be played with by pulling the code from my branch here. What it does is helps one connect an Operator backed service to a odo component. One can see the environment variables getting injected in the pod of the application. However, this is not of much use since the application itself doesn't succeed in coming up.

Problem

The way this "injection of environment variables" happens is by modifying the Deployment of the component (a.k.a. application). This causes the underlying pod to be terminated and a new one is spun up which contains the environment variable(s).

Presently, for devfile components, odo uses emptyDir volumes to store the code. This doesn't persist a reboot and, as a result, the application in the pod doesn't come up.

A possible solution

Store the information about the link (ServiceBindingRequest) in the .odo/env/env.yaml file. Here "information" is the name of the Secret created as a result of creating a ServiceBindingRequest to link component & service.

Upon a successful odo link, we tell the user that they need to do odo push for linking to take effect. When they do odo push, odo will create a Deployment with the information added as envFrom field in the Deployment's YAML.

Another solution (probably for a longer term) could be to use some persistent form of storage to store the code in the pod instead of using emptyDir volume.

@kadel please correct me if I'm mistaken in summarizing our discussion.

[dharmit]

Store the information about the link (ServiceBindingRequest) in the .odo/env/env.yaml file. Here "information" is the name of the Secret created as a result of creating a ServiceBindingRequest to link component & service.

We need to decide how we would like to store this information in the env.yaml file.

Do we want to make it a part of ComponentSettings struct in envinfo.go file: https://github.com/openshift/odo/blob/b1d8462e7bce01e41e24e2e853d4203e32d91408/pkg/envinfo/envinfo.go#L18-L26

If yes, what pieces of information do we want to store? Here's a few that I can think of:

• Name of the secret created on the cluster as a result of creation of SBR. This, in my observation, has the same name as that of the SBR itself.
• Type of object being linked through the SBR: either a component or a service (since we would like odo link to support linking a component with another component or an Operator backed service)
• Name of the object (a component or a service) being linked through the SBR

However, I'm sceptical about how things will work out when we try to link with a Service Catalog backed service.

Upon a successful odo link, we tell the user that they need to do odo push for linking to take effect. When they do odo push, odo will create a Deployment with the information added as envFrom field in the Deployment's YAML.

This is the approach that we thought would be helpful in averting the problem mentioned in #2920 (comment). If there's any other approach that anyone would like to share, please mention.

@kadel @girishramnani @mik-dass @adisky @cdrage

[girishramnani]

Is this an upstream issue? Like the pod shouldnt be recreated due to just injection of env variables.

[dharmit]

Is this an upstream issue? Like the pod shouldnt be recreated due to just injection of env variables.

The deployment's definition changes as a result of the injection of environment variable. And hence the underlying pod's going to be recreated. It's more of design than an issue.