Create service binding through topology connector #3065

divyanshiGupta · 2019-10-23T11:57:49Z

Jira Story - https://jira.coreos.com/browse/ODC-1978

This PR adds following functionalities -

Allow service binding only when service binding operator is installed.
Create service binding between nodes if the target node is an operator-backed-service otherwise create pseudo binding.
Remove service binding.

To Do -

Add visual changes to distinguish between service binding and pseudo binding connector
Add visual changes to depict service binding failure
cc: @serenamarie125

Note -
Removing connects-to label from source app and deleting the SBR doesn't trigger re-deployment of the source app. @sbose78 is there a way we can do that?

divyanshiGupta · 2019-10-23T11:59:41Z

/cc @christianvogt

divyanshiGupta · 2019-10-23T17:58:40Z

/cc @rohitkrai03

divyanshiGupta · 2019-10-23T19:09:32Z

/test e2e-gcp-console

divyanshiGupta · 2019-10-23T19:09:53Z

/cc @serenamarie125

divyanshiGupta · 2019-10-23T19:22:41Z

/test e2e-gcp-console

sbose78 · 2019-10-23T20:03:40Z

Removing connects-to label from source app and deleting the SBR doesn't trigger re-deployment of the source app. @sbose78 is there a way we can do that?

Sure, we can. CC / @otaviof @akashshinde

christianvogt · 2019-10-24T01:10:52Z

frontend/packages/console-shared/src/utils/resource-utils.ts

+      isList: true,
+      kind: referenceForModel(ClusterServiceVersionModel),
+      namespace,
+      prop: 'clusterServiceVersion',


Why is this in console shared? Since this is only used by topology we shouldn't be fetching this for the overview.

What happens if this resource isn't available? Probably want this optional or feature flagged.

Cc @invincibleJai

christianvogt · 2019-10-24T01:15:37Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

  return {
    id: dcUID,
    name:
      _.get(deploymentConfig, 'metadata.name') || deploymentsLabels['app.kubernetes.io/instance'],
    type: 'workload',
    resources: { ...dc },
    pods: dc.pods,
+    operatorBackedService: !!_.find(operatorBackedServiceKinds, (k) => {


Use includes

christianvogt · 2019-10-24T01:31:28Z

frontend/packages/dev-console/src/utils/application-utils.ts

+    apiVersion: 'apps.openshift.io/v1alpha1',
+    kind: 'ServiceBindingRequest',
+    metadata: {
+      name: `${sourceName}-${targetName}-sbr`,


Should probably use a random string suffix to lessen the chance of failure.

christianvogt · 2019-10-24T01:41:40Z

frontend/packages/dev-console/src/utils/application-utils.ts

+        matchLabels: { ...applicationSelectorLabels },
+        group: 'apps.openshift.io',
+        version: 'v1',
+        resource: 'deploymentconfigs',


What about the other workload types? Eg deployment

Note, Any deployment, deploymentconfig and knativeService is supported.

This section must be generated based on the source resource backing the topology node.

Thanks for pointing this out. Almost forgot about making the resource type generic.

christianvogt · 2019-10-24T02:11:39Z

frontend/packages/dev-console/src/utils/application-utils.ts

+    },
+    spec: {
+      applicationSelector: {
+        matchLabels: { ...applicationSelectorLabels },


Since this service binding is named specifically for binding the given source and target, shouldn't the selector be very specific to the target? I think the connects-to convention was intended to create a single service binding that can bind multiple apps to the service.

What happens when we have many service binding request resources that bind to the same service using the same selector?

If we intend to create a specific binding per source and target, it may be better to create a selector against the recommended labels of the source.

Thoughts @sbose78 ?
Is there only a selector? Or also a resource ref?

You could use resourceRef too.

apiVersion: apps.openshift.io/v1alpha1 kind: ServiceBindingRequest metadata: name: binding-request namespace: service-binding-demo spec: applicationSelector: resourceRef: nodejs-app group: apps.openshift.io version: v1 resource: deploymentconfigs backingServiceSelector: resourceRef: db-demo group: postgresql.baiju.dev version: v1alpha1 kind: Database

Awesome!
I think it makes more sense to create very targeted SBR in this case.

christianvogt · 2019-10-24T02:14:09Z

frontend/packages/dev-console/src/utils/application-utils.ts

@@ -44,6 +47,20 @@ export const edgesFromAnnotations = (annotations): string[] => {
  return edges;
 };

+export const edgesFromLabels = (labels): string[] => {


This assumes that by having a connects-to label that the app is bound to a service. That isn't always true. We should be getting the service binding requests and matching the selector to workloads. The connects-to label selector is a convention however any selector can be used. Then our topology wouldn't show the app binding.

Further to this, we need a resource backing the edge itself so that we can select the edge and display the SBR in the side bar.

christianvogt · 2019-10-24T02:20:51Z

frontend/packages/dev-console/src/utils/application-utils.ts

+      },
+      backingServiceSelector: {
+        group: targetResourceGroup[0],
+        version: 'v1alpha1',


This cannot be hard coded.

Yes, this information is available in the csv - example, https://github.com/operator-framework/community-operators/blob/master/community-operators/apicurito/0.1.0/apicuritooperator.v0.1.0.clusterserviceversion.yaml#L27

christianvogt · 2019-10-24T02:32:08Z

frontend/packages/dev-console/src/plugin.tsx

@@ -50,6 +51,13 @@ const plugin: Plugin<ConsumedExtensions> = [
      flag: SHOW_PIPELINE,
    },
  },
+  {
+    type: 'FeatureFlag/Model',


I don't see the flag being used. But it should probably.
We'll need feature checks to enable SBR feature as well as rbac checks

Oops missed to add a check for service binding operator before allowing the binding. My bad :(

christianvogt · 2019-10-25T15:01:45Z

frontend/packages/dev-console/src/components/topology/TopologyDataController.tsx

+  resources.push(
+    {
+      isList: true,
+      kind: referenceForModel(ClusterServiceVersionModel),
+      namespace,
+      prop: 'clusterServiceVersion',
+    },
+    {
+      isList: true,
+      kind: referenceForModel(ServiceBindingRequestModel),
+      namespace,
+      prop: 'servicebindingrequests',
+    },
+  );


You're fetching these resources even if they do not exist. This can cause an error just like we encountered with knative. We need to only include these resources if they are installed and if we have read access. So using optional: true here seems like it would solve the issue. Additionally don't include if serviceBinding === false

christianvogt · 2019-10-25T15:04:58Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

  const { obj: deploymentConfig, current, previous, isRollingOut } = dc;
  const dcUID = _.get(deploymentConfig, 'metadata.uid');
  const deploymentsLabels = _.get(deploymentConfig, 'metadata.labels', {});
  const deploymentsAnnotations = _.get(deploymentConfig, 'metadata.annotations', {});
  const { buildConfigs } = dc;
+  const nodeResourceKind = _.get(deploymentConfig, 'metadata.ownerReferences[0].kind');


Is the first ownerReference sufficient?

Yes, as per this conversation here - https://coreos.slack.com/archives/CHGU4P8UU/p1571750071352200?thread_ts=1571748334.342700&cid=CHGU4P8UU.

christianvogt · 2019-10-25T15:31:15Z

frontend/packages/dev-console/src/utils/application-utils.ts

+  const edges: string[] = [];
+  _.forEach(sbrs, (sbr) => {
+    let edgeExists = false;
+    if (_.get(sbr, 'spec.applicationSelector.resourceRef') === source.metadata.name) {


You also need to match the other properties like resource to ensure it's the right kind.

christianvogt · 2019-10-25T15:32:19Z

frontend/packages/dev-console/src/utils/application-utils.ts

+        }
+      }
+    }
+    edgeExists && edges.push(sbr.spec.backingServiceSelector.resourceRef);


You need more than the name here. Again you need the kind.

christianvogt · 2019-10-25T15:35:13Z

frontend/packages/dev-console/src/utils/application-utils.ts

+    },
+  };
+
+  return checkServiceBindingPermissions(namespace, 'delete', sbrName)


The check here isn't really useful because the operation would have failed anyways if they didn't have permission. We should not be allowing the user to perform the operation in the first place. I say remove the rbac checks here for now. When we move to the new topology it'll be easier to add rbac to prevent the user from seeing the delete icon in the first place.

christianvogt · 2019-10-25T15:39:11Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

+    if (targetNode) {
+      edges.push({
+        id: `${uid}_${targetNode}`,
+        type: 'connects-to',


This should be of type service-binding.
You need to identify the edge type being deleted to call the right delete function.
For now you can contribute the same edge shape inside shape-providers.ts for this new type.

christianvogt · 2019-10-25T15:41:19Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

+  if (target.operatorBackedService) {
+    return removeServiceBinding(sourceObj, targetObj);
+  }
+
  return removeResourceConnection(sourceObj, targetObj);


You need to check which is the edge.type being deleted here.
Update the following inside D3ForceDirectedRenderer to also include the edge type. If connects-to do the old delete, if service-binding do the new delete.
onRemove={() => onRemoveConnection(viewEdge.source.id, viewEdge.target.id)}

christianvogt · 2019-10-26T13:01:23Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

+    );
+    const uid = _.get(dc, ['metadata', 'uid']);
+    if (targetNode) {
+      edges.push({


Save the trouble of lookup when you delete the edge by storing the SBR in the data prop of the edge.

Yes, initially I also thought of doing that but I was not sure if it makes sense to store the SBR in the edge data itself.

christianvogt · 2019-10-26T13:02:48Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

+    const targetNode = _.get(
+      _.find(resources, (deployment) => {
+        const targetFound =
+          _.get(deployment, 'metadata.ownerReferences[0].name') === edge.resourceRef &&


Why is this not a comparison against the name and kind of the resource? Why the ownerReference?

Looking at the examples it seems that they are probably identifying which service to bind to based on the service name and its kind for eg Database etc and not based on its Deployment.

Service

kind: Database metadata: name: db-demo namespace: service-binding-demo spec: image: docker.io/postgres imageName: postgres dbName: db-demo

SBR

apiVersion: apps.openshift.io/v1alpha1 kind: ServiceBindingRequest metadata: name: binding-request namespace: service-binding-demo spec: applicationSelector: matchLabels: connects-to: postgres environment: demo group: apps.openshift.io version: v1 resource: deploymentconfigs backingServiceSelector: group: postgresql.baiju.dev version: v1alpha1 kind: Database resourceRef: db-demo

christianvogt · 2019-10-26T13:07:43Z

frontend/packages/dev-console/src/utils/application-utils.ts

+    spec: {
+      applicationSelector: {
+        resourceRef: sourceName,
+        group: 'apps.openshift.io',


This is the wrong group for a deployment.
This field needs to be dynamic based on the source resource.

christianvogt · 2019-10-26T13:08:14Z

frontend/packages/dev-console/src/utils/application-utils.ts

+
+  const targetResourceKind = _.get(target, 'metadata.ownerReferences[0].kind');
+  const targetResourceRefName = _.get(target, 'metadata.ownerReferences[0].name');
+  const removeSbr = _.find(sbrs, (sbr) => {


This can be avoided if you store the SBR in the edge data.

christianvogt · 2019-10-28T18:58:46Z

frontend/packages/dev-console/src/utils/application-utils.ts

+  if (!source || !target || source === target) {
+    return Promise.reject();
+  }


I don't think this matters really. If you got the sbr, go ahead and delete it

christianvogt · 2019-10-28T19:00:04Z

/approve

christianvogt · 2019-10-28T19:53:56Z

/lgtm

openshift-bot · 2019-10-28T21:53:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-29T03:06:21Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-29T03:32:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

rohitkrai03 · 2019-10-29T06:28:34Z

/lgtm

openshift-ci-robot · 2019-10-29T06:28:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, rohitkrai03

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:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt,rohitkrai03]

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

rohitkrai03 · 2019-10-29T06:44:58Z

/test e2e-gcp-console

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console component/shared Related to console-shared labels Oct 23, 2019

openshift-ci-robot requested review from gijohn and kyoto October 23, 2019 11:58

openshift-ci-robot requested a review from christianvogt October 23, 2019 11:59

openshift-ci-robot requested a review from rohitkrai03 October 23, 2019 17:58

openshift-ci-robot requested a review from serenamarie125 October 23, 2019 19:09

christianvogt reviewed Oct 24, 2019

View reviewed changes

christianvogt reviewed Oct 25, 2019

View reviewed changes

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 26, 2019

divyanshiGupta requested a review from christianvogt October 26, 2019 09:41

christianvogt reviewed Oct 26, 2019

View reviewed changes

divyanshiGupta requested a review from christianvogt October 26, 2019 20:18

christianvogt reviewed Oct 28, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2019

divyanshiGupta closed this Oct 28, 2019

divyanshiGupta reopened this Oct 28, 2019

openshift-ci-robot assigned christianvogt Oct 28, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2019

divyanshiGupta added 6 commits October 29, 2019 09:07

add feature flag for sbr operator

8380557

add create service binding functionality

dbef798

add support for identifying if a service is operator backed

32583b8

add remove service binding functionality

00c54f2

add checks for service binding access and service binding operator

c11282a

create edges based on sbrs

0f66cb5

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

openshift-ci-robot assigned rohitkrai03 Oct 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

openshift-merge-robot merged commit 197b678 into openshift:master Oct 29, 2019

spadgett added this to the v4.3 milestone Oct 31, 2019

Create service binding through topology connector #3065

Create service binding through topology connector #3065

Conversation

divyanshiGupta commented Oct 23, 2019 • edited Loading

divyanshiGupta commented Oct 23, 2019

divyanshiGupta commented Oct 23, 2019

divyanshiGupta commented Oct 23, 2019

divyanshiGupta commented Oct 23, 2019

divyanshiGupta commented Oct 23, 2019

sbose78 commented Oct 23, 2019

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

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

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

christianvogt commented Oct 28, 2019

christianvogt commented Oct 28, 2019

openshift-bot commented Oct 28, 2019

openshift-bot commented Oct 29, 2019

openshift-bot commented Oct 29, 2019

rohitkrai03 commented Oct 29, 2019

openshift-ci-robot commented Oct 29, 2019

rohitkrai03 commented Oct 29, 2019

divyanshiGupta commented Oct 23, 2019 •

edited

Loading