Add support for creating component from service catalog #413

surajnarwade · 2018-04-24T07:35:49Z

Fixes #266

Signed-off-by: Suraj Narwade surajnarwade353@gmail.com

This PR adds the support for creating services from service catalog. for this support, we have used
client from kubernetes-incubator/service-catalog.
Currently, we are supporting template service broker only. In future, we will add support for
other brokers as well.

We have added entire new command set for this purpose.

to check available services in service catalog

$ odo service catalog

to create service,

$ odo service create mysql-persistent

to delete service,

$ odo service delete mysql-persistent

to list the current provisioned services with their status

$ odo service list

kadel · 2018-04-24T09:11:14Z

please add link to the issue

surajnarwade · 2018-04-30T13:35:02Z

Fixes #266

surajnarwade · 2018-04-30T13:37:50Z

Prerequisite to run/test this PR:

Run following commands,

minishift config set openshift-version v3.7.1

export MINISHIFT_ENABLE_EXPERIMENTAL=y

minishift config set iso-url centos

minishift start --service-catalog

Enable ansible-service-broker on minishift, refer https://github.com/minishift/minishift-addons/tree/master/add-ons/ansible-service-broker#deploy-the-ansible-service-broker

kadel · 2018-04-30T14:40:03Z

pkg/catalog/catalog.go

@@ -9,55 +9,68 @@ import (
 )

 // List lists all the available component types
-func List(client *occlient.Client) ([]string, error) {
+func List(client *occlient.Client) (finalList map[string][]string, err error) {


please don't use named return values. It makes it hard to read :-(

kadel · 2018-04-30T14:41:25Z

pkg/catalog/catalog.go

 	}

 	return result, nil
 }

 // Exists returns true if the given component type is valid, false if not
-func Exists(client *occlient.Client, componentType string) (bool, error) {
+func Exists(client *occlient.Client, componentType string) (bool, error, string) {


Please add a comment explaining what return values are. Especially new returned string

kadel · 2018-04-30T14:49:50Z

pkg/occlient/occlient.go

+func (c *Client) CreateServiceInstance(componentName string, componentType string, labels map[string]string, applicationName string, projectName string) error {
+	// Patching Template with ODO specific labels
+	labelPatch := fmt.Sprintf(`{"labels": {"app.kubernetes.io/component-name": "%s", "app.kubernetes.io/component-type": "%s", "app.kubernetes.io/name": "%s"}}`, componentName, componentType, applicationName)
+	_, err := c.templateClient.Templates("openshift").Patch(componentType, types.StrategicMergePatchType, []byte(labelPatch))


won't this permanently modify templates saved in openshift namespace?

what if service is from some other broker than openshift template broker?

I didn't get second question

The function is called CreateServiceInstance, but is using OpenShift templates.
It is really confusing :-(
There can be multiple different Service brokers in Service Catalog.

kadel · 2018-04-30T14:50:51Z

pkg/occlient/occlient.go

@@ -892,6 +899,21 @@ func (c *Client) GetClusterServiceClasses() ([]scv1beta1.ClusterServiceClass, er
 	return classList.Items, nil
 }

+func (c *Client) CreateServiceInstance(componentName string, componentType string, labels map[string]string, applicationName string, projectName string) error {


we are in occlient package it should have componentName or componentType variables

kadel · 2018-04-30T14:52:18Z

pkg/occlient/occlient.go

+
+	"github.com/pkg/errors"
+	log "github.com/sirupsen/logrus"
+	types "k8s.io/apimachinery/pkg/types"


imports should be logically grouped together

that was auto goimports in goland :(

kadel · 2018-04-30T14:55:21Z

cmd/create.go

@@ -127,6 +127,10 @@ A full list of component types that can be deployed is available using: 'odo com
 			checkError(err, fmt.Sprintf("failed to push component: %v", componentName))

 			fmt.Printf("Component '%s' was created.\n", componentName)
+		} else if len(catalogType) != 0 && catalogType == "template" {


what if len(catalogType) != 0 and catalogType is different from "template"?

other catalog type is imagestream so other conditions will handle that

kadel · 2018-04-30T15:02:57Z

It breaks for everything if cluster doesn't have service catalog.
It this case regular s2i should work.

▶ odo create wildfly
User "developer" cannot list clusterserviceclasses.servicecatalog.k8s.io at the cluster scope: User "developer" cannot list all clusterserviceclasses.servicecatalog.k8s.io in the cluster (get clusterserviceclasses.servicecatalog.k8s.io)

kadel · 2018-04-30T15:03:39Z

minishift config set openshift-version v3.7.1

why is 3.7.1 required?

minishift config set iso-url centos

why is centos required?

surajnarwade · 2018-04-30T15:06:07Z

@kadel , service catalog & ansible service broker addons works well with this version

kadel · 2018-04-30T15:16:10Z

pkg/occlient/occlient.go

+		return errors.Wrap(err, "unable to patch template")
+	}
+	// Creating Service Instance
+	_, err = c.serviceCatalogClient.ServiceInstances(projectName).Create(&scv1beta1.ServiceInstance{metav1.TypeMeta{Kind: "ServiceInstance", APIVersion: "servicecatalog.k8s.io/v1beta1"}, metav1.ObjectMeta{Finalizers: []string{"kubernetes-incubator/service-catalog"}, Name: componentName, Namespace: "myproject", Labels: labels}, scv1beta1.ServiceInstanceSpec{UserInfo: &scv1beta1.UserInfo{Username: "developer"}, PlanReference: scv1beta1.PlanReference{ClusterServiceClassExternalName: componentType}}, scv1beta1.ServiceInstanceStatus{}})


please add break ServiceInstance struct to multiple lines,it is hard to see what is going on there.

You are passing labels there. If you can pass labels, why is the patching needed in the previous step?

oh I am gonna remove this label, I was just playing around it. this label will only apply to serviceinstance. But we want label on every resource that will be created for example, deploymentconfig, services, routes

kadel · 2018-04-30T15:16:41Z

pkg/occlient/occlient.go

@@ -892,6 +899,21 @@ func (c *Client) GetClusterServiceClasses() ([]scv1beta1.ClusterServiceClass, er
 	return classList.Items, nil
 }

+func (c *Client) CreateServiceInstance(componentName string, componentType string, labels map[string]string, applicationName string, projectName string) error {
+	// Patching Template with ODO specific labels
+	labelPatch := fmt.Sprintf(`{"labels": {"app.kubernetes.io/component-name": "%s", "app.kubernetes.io/component-type": "%s", "app.kubernetes.io/name": "%s"}}`, componentName, componentType, applicationName)


please break this to multiple lines

kadel

I'm not able to create from catalog

▶ odo create jenkins-persistent
User "developer" cannot patch templates.template.openshift.io in the namespace "openshift": User "developer" cannot "patch" "templates.template.openshift.io" with name "jenkins-persistent" in project "openshift" (patch templates.template.openshift.io jenkins-persistent)

anmolbabu · 2018-08-27T12:07:35Z

cmd/service.go

+	},
+}
+
+//var serviceCatalogCmd = &cobra.Command{


Why Commented out?
I tried this based on #413 (comment) but doesn't seem to be working

surajnarwade · 2018-08-27T13:43:25Z

Commands to test:

* odo catalog list components

* odo catalog list services

* odo catalog search components

* odo catalog search services

* odo service create <service_name>

* odo service delete <service_name>

* odo service list

cc @anmolbabu @cdrage @mik-dass

jorgemoralespou · 2018-08-27T15:57:28Z

I would add to the list of tests the ones for a component, as the possible components that can be used are from the catalog.

* odo create <component_type> <component_name>

* odo delete <component_name>

* odo list

anmolbabu

Pleas also add e2e tests

I tried to setup minishift with service catalog enabled but I see consistent failures with it :

sometimes at the time of pulling minishift-centos iso
sometimes at the time of pulling origin container image
sometimes at time of starting openshift cluster

So, couldn't verify the PR

anmolbabu · 2018-08-28T06:35:32Z

cmd/catalog.go

+	Run: func(cmd *cobra.Command, args []string) {
+		client := getOcClient()
+		catalogList, err := svc.ListCatalog(client)
+		checkError(err, "unable to list services because Service Catalog is not enabled in your cluster")


would it better to only say "unable to list services" and leave the other part of the error to come from openshift?
If there's a network error for example, I feel it might be misleading to suggest "Service Catalog is not enabled in your cluster"

our user will not understand openshift error (so we have to simplify it)
if there is network error, process will not reach till this stage :P

Hmm so the occlient creation would fail is what you are saying right? So can there be any other error like permissions/authorisation for using service catalog issue ?

If not please ignore this comment

I think this error message is okay for this situation

anmolbabu · 2018-08-28T10:57:19Z

pkg/occlient/occlient.go

+	// Service Instance
+	glog.V(4).Infof("Deleting Service Instance")
+	// Listing out serviceInstance because `DeleteCollection` method don't work on serviceInstance
+	svcCatList, err := c.serviceCatalogClient.ServiceInstances(c.namespace).List(metav1.ListOptions{LabelSelector: selector})


How is this different from https://github.com/redhat-developer/odo/pull/413/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR1204 can it be reused here?

anmolbabu · 2018-08-28T10:58:21Z

pkg/occlient/occlient.go

 	return values, nil
 }

+// GetServiceInstanceList returns list service instances
+func (c *Client) GetServiceInstanceList(project string, selector string) ([]scv1beta1.ServiceInstance, error) {


should we call project as namespace ?
Just trying to avoid multiple terminologies for same thing

this is fine, I guess

anmolbabu · 2018-08-28T11:09:11Z

pkg/occlient/occlient_test.go

+			if !reflect.DeepEqual(createdServiceInstance.Labels, tt.args.labels) {
+				t.Errorf("labels in created serviceInstance is not matching expected labels, expected: %v, got: %v", tt.args.labels, createdServiceInstance.Labels)
+			}
+			if !reflect.DeepEqual(createdServiceInstance.Name, tt.args.componentName) {


Can we have just == comparisons for basic types ?

anmolbabu · 2018-08-28T11:15:43Z

pkg/service/service.go

@@ -0,0 +1,133 @@
+package service


+1
Doesn't look like a lot of validations in this file
But, may be for code-coverage :)

anmolbabu

LGTM

But may be you need to add unit test for pkg/service/service.go + also e2e tests

cmoulliard

odo service create mysql-persistent

I suppose that you assume here that mysql-persistent corresponds to the class of the service to be used to create a ServiceInstance ? How will the user be able to define the serviceInstance name to be used ? How will it be possible to define the parameters of the service (e.g. DB_*** parameters) ? @surajnarwade

cmoulliard · 2018-08-27T13:52:42Z

pkg/occlient/occlient.go

 func (c *Client) GetClusterServiceClasses() ([]scv1beta1.ClusterServiceClass, error) {
-	classList, err := c.serviceCatalogClient.ClusterServiceClasses().List(metav1.ListOptions{})
+	// TODO: Remove `FieldSelector` from ListOptions when we are confident with Ansible Service Broker
+	classList, err := c.serviceCatalogClient.ClusterServiceClasses().List(metav1.ListOptions{FieldSelector: "spec.clusterServiceBrokerName=template-service-broker"})


OSB is working pretty well for me

minishift v1.23.0+91235ee minishift config set memory 6G minishift config set openshift-version v3.10.0 minsishift addons enable admin-user MINISHIFT_ENABLE_EXPERIMENTAL=y minishift start --extra-clusterup-flags="--enable=*,service-catalog,automation-service-broker"

cmoulliard · 2018-08-27T13:54:16Z

pkg/occlient/occlient.go

 	if err != nil {
 		return nil, errors.Wrap(err, "unable to list cluster service classes")
 	}
 	return classList.Items, nil
 }

+// CreateServiceInstance creates service instance from service catalog
+func (c *Client) CreateServiceInstance(componentName string, componentType string, labels map[string]string) error {


I think that your createServiceInstance function doesn't include the spec.parameters

Example : https://github.com/snowdrop/cloud-native-backend/blob/master/openshift/mysql_serviceinstance.yml#L9-L13

@surajnarwade

jorgemoralespou · 2018-08-29T08:04:31Z

odo service create mysql-persistent without the required parameters should yield an error, and user would need to provide them.

Typically every service will require some configuration, so user will need to odo service describe <service_name> to get the parameter configuration so that the user can create the service.

In reality this will get horribly complex because:

When creating a service, one might need to select a plan
When creating a service, externalName is not what defines the service id to use, but it's just a text that can be repeated multiple times.

This task it's turning more complex than initially envisioned for the way how servicecatalog API works. I would defer the work or think that we should revisit this in the near future with many changes.
cc/ @kadel

surajnarwade · 2018-08-29T08:44:02Z

@jorgemoralespou , @cmoulliard :

This PR just demonstrate that we can create the basic service from service catalog. Also, this PR does not conflict with odo's core functionality. Upon merging this PR:

Next possible steps are:

Add respective plans and parameters to odo catalog list services
Modify odo service create service_name to accept plans and parameters, either by arguments or interactive mode
Upon running odo link <target_component> --service <service_name> will inject secret into env of deploymentconfig and restart it

WDYT ?

cmoulliard · 2018-08-29T09:13:15Z

will get horribly complex because

I agree with you. The flow requires that we do :

Search about a service and plan
Extract from the plan the parameters and customize them (-> under MANIFEST or application.yml file maybe)
(out of scope of odo) Select the correct maven dependency to add. For spring Boot -> starter, cloud connector able to process data of the service
Create a service instance using parameters customized
Generate a Secret with the parameters (= binding)
Update the DeploymentConfig to add the secret as EnvFrom
Rollout the DeploymentConfig

jorgemoralespou · 2018-08-29T09:23:07Z

@surajnarwade sounds good to me.

@cmoulliard

Search about a service and plan

odo catalog list service
odo catalog search service

Extract from the plan the parameters and customize them (-> under MANIFEST or application.yml file maybe)

odo catalog describe service

Create a service instance using parameters customized

odo service create -p param1=value1 -p param2=value2

Generate a Secret with the parameters (= binding)
Update the DeploymentConfig to add the secret as EnvFrom
Rollout the DeploymentConfig

odo link

anmolbabu

Please create a bug to track e2e test and docs only so that we don't lose track of it and link back here

cmoulliard · 2018-08-29T12:57:54Z

odo service create -p param1=value1 -p param2=value2

this command will work for service containing string's fields but when json or yaml content is required (as this is the case for the keycloak's service), then such info should be defined within a config's application file (MANIFEST, odo-application.yaml)

mik-dass

LGTM

surajnarwade · 2018-08-29T13:10:02Z

@jorgemoralespou @cmoulliard @cdrage , if you all agree, then @anmolbabu can merge this PR
(since we got 3 LGTMs :D )

jorgemoralespou · 2018-08-29T14:40:46Z

@surajnarwade I can't vote as I can't read the implementation. To me looks good and as part of the discussion we can create follow up issues.

anmolbabu · 2018-08-29T14:48:52Z

Merging based on previous acks but @surajnarwade please log issue to add documentation and e2e tests

anmolbabu · 2018-08-29T15:26:44Z

As per our discussion, I have also raised #678 please take it up on priority @surajnarwade

This reverts commit bbcf73f.

cdrage · 2018-08-29T14:38:43Z

cmd/catalog.go

@@ -21,15 +22,26 @@ var catalogCmd = &cobra.Command{

 var catalogListCmd = &cobra.Command{
 	Use:   "list",
+	Short: "List all available component & service types.",


I think you should change the sentence so both commands are similar:

Currently it is:

Available Commands: components List all available component types. services Lists all the services from service catalog

Maybe: Lists all components available
and Lists all services available ?

cdrage · 2018-08-29T14:41:00Z

cmd/catalog.go

+	Example: `  # List all services
+  odo catalog list services
+
+ # Search for a supported services


Search for a supported service

cdrage · 2018-08-29T15:36:37Z

cmd/service.go

+
+If service name is not provided, service type value will be used.
+
+A full list of service types that can be deployed is available using: 'odo service catalog'`,


are available

cdrage · 2018-08-29T15:37:10Z

cmd/service.go

+		exists, err = svc.SvcExists(client, serviceName, applicationName, projectName)
+		checkError(err, "")
+		if exists {
+			fmt.Printf("Service with the name %s already exists in the current application.\n", serviceName)


remove with

cdrage · 2018-08-29T15:37:35Z

cmd/service.go

+	Use:   "delete <service_name>",
+	Short: "Delete an existing service",
+	Long:  "Delete an existing service",
+	Example: `  # Delete service named 'mysql-persistent'


Delete the service

cdrage · 2018-08-29T15:47:59Z

cmd/service.go

+	serviceCmd.SetUsageTemplate(cmdUsageTemplate)
+	serviceCmd.AddCommand(serviceCreateCmd)
+	serviceCmd.AddCommand(serviceDeleteCmd)
+	//serviceCmd.AddCommand(serviceCatalogCmd)


remove commented out code

cdrage · 2018-08-29T15:55:45Z

pkg/occlient/occlient.go

+	glog.V(4).Infof("Selectors used for deletion: %s", selector)
+	// Service Instance
+	glog.V(4).Infof("Deleting Service Instance")
+	// Listing out serviceInstance because `DeleteCollection` method don't work on serviceInstance


add some newline / spacing :( makes everything a bit cluttered up here.

cdrage · 2018-08-29T15:55:53Z

pkg/occlient/occlient.go

+	// convert labels to selector
+	selector := util.ConvertLabelsToSelector(labels)
+	glog.V(4).Infof("Selectors used for deletion: %s", selector)
+	// Service Instance


do we really need this comment

cdrage · 2018-08-29T15:57:56Z

pkg/occlient/occlient.go

 	if err != nil {
 		return nil, errors.Wrap(err, "unable to list cluster service classes")
 	}
 	return classList.Items, nil
 }

+// CreateServiceInstance creates service instance from service catalog
+func (c *Client) CreateServiceInstance(componentName string, componentType string, labels map[string]string) error {


@surajnarwade

cdrage · 2018-08-29T16:00:44Z

pkg/service/service.go

+	//since, service is associated with application, it consist of application label as well
+	// which we can give as a selector
+	applicationSelector := util.ConvertLabelsToSelector(labels)
+	// get service instance list based on given selector


add newline / spacing

surajnarwade changed the title ~~Add support for creating component from service catalog~~ [WIP]Add support for creating component from service catalog Apr 24, 2018

surajnarwade added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Apr 24, 2018

surajnarwade force-pushed the catalog branch from 87a49d2 to a7a339d Compare April 25, 2018 15:09

surajnarwade added the do not review label Apr 25, 2018

surajnarwade force-pushed the catalog branch 4 times, most recently from 9c919a6 to 9c58fa1 Compare April 30, 2018 13:34

surajnarwade removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do not review do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Apr 30, 2018

surajnarwade changed the title ~~[WIP]Add support for creating component from service catalog~~ Add support for creating component from service catalog Apr 30, 2018

kadel reviewed Apr 30, 2018

View reviewed changes

kadel requested changes Apr 30, 2018

View reviewed changes

surajnarwade force-pushed the catalog branch 2 times, most recently from ab1de12 to 1153b25 Compare May 2, 2018 08:46

updated component/service search in catalog

c980f25

surajnarwade force-pushed the catalog branch from f41cd39 to c980f25 Compare August 27, 2018 07:30

anmolbabu reviewed Aug 27, 2018

View reviewed changes

anmolbabu reviewed Aug 28, 2018

View reviewed changes

remove commented code from service.go

d04a0c2

surajnarwade force-pushed the catalog branch from 1c39331 to d04a0c2 Compare August 28, 2018 12:15

removed hard coded template-service-broker

0d8eb1d

surajnarwade changed the title ~~Add support for creating component from service catalog~~ [WIP]Add support for creating component from service catalog Aug 28, 2018

surajnarwade added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. labels Aug 28, 2018

surajnarwade changed the title ~~[WIP]Add support for creating component from service catalog~~ Add support for creating component from service catalog Aug 28, 2018

anmolbabu reviewed Aug 28, 2018

View reviewed changes

cmoulliard reviewed Aug 29, 2018

View reviewed changes

anmolbabu approved these changes Aug 29, 2018

View reviewed changes

surajnarwade mentioned this pull request Aug 29, 2018

Add CLI-reference related to service catalog #677

Merged

mik-dass approved these changes Aug 29, 2018

View reviewed changes

anmolbabu merged commit bbcf73f into redhat-developer:master Aug 29, 2018

cdrage added a commit that referenced this pull request Aug 29, 2018

Revert "Add support for creating component from service catalog (#413)"

0d89e9f

This reverts commit bbcf73f.

cdrage reviewed Aug 29, 2018

View reviewed changes


		If service name is not provided, service type value will be used.

		A full list of service types that can be deployed is available using: 'odo service catalog'`,

Add support for creating component from service catalog #413

Add support for creating component from service catalog #413

Conversation

surajnarwade commented Apr 24, 2018 • edited by cdrage

kadel commented Apr 24, 2018

surajnarwade commented Apr 30, 2018

surajnarwade commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Apr 30, 2018 • edited

Choose a reason for hiding this comment

kadel Apr 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Apr 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Apr 30, 2018

kadel commented Apr 30, 2018 • edited

surajnarwade commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

anmolbabu Aug 27, 2018 • edited

Choose a reason for hiding this comment

surajnarwade commented Aug 27, 2018

jorgemoralespou commented Aug 27, 2018

anmolbabu left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu left a comment

Choose a reason for hiding this comment

cmoulliard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgemoralespou commented Aug 29, 2018

surajnarwade commented Aug 29, 2018 • edited

cmoulliard commented Aug 29, 2018

jorgemoralespou commented Aug 29, 2018

anmolbabu left a comment

Choose a reason for hiding this comment

cmoulliard commented Aug 29, 2018

mik-dass left a comment

Choose a reason for hiding this comment

surajnarwade commented Aug 29, 2018

jorgemoralespou commented Aug 29, 2018

anmolbabu commented Aug 29, 2018

anmolbabu commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Apr 24, 2018 •

edited by cdrage

kadel Apr 30, 2018 •

edited

kadel Apr 30, 2018 •

edited

kadel Apr 30, 2018 •

edited

kadel commented Apr 30, 2018 •

edited

anmolbabu Aug 27, 2018 •

edited

anmolbabu left a comment •

edited

surajnarwade commented Aug 29, 2018 •

edited