feat: Add ServiceBinding trait (#1445) #1952

johnpoth · 2021-01-27T17:08:47Z

kamel run MyDbIntegration.java --connect Database.version.group/my-postgresql

Thanks !

Release Note

[ServiceBinding](https://github.com/k8s-service-bindings/spec) support has been added via the ServiceBinding trait

nicolaferraro

It sounds really good that the binding is done operator side and that the complexity is delegated to the runtime, where we know we can later leverage Quarkus OOB support.

I've just a few comments about issues that can happen when servicebinding is not installed in the cluster (or it's installed later).

I think you also forgot to add something like this thing we did with Strimzi to install the role and cluster role (for kamel install), plus adding the rules to OLM and HELM installation.

After this is merged, we can think to add a specific E2E test on a cluster with ServiceBinding installed to see how it goes.

nicolaferraro · 2021-01-28T09:18:29Z

pkg/controller/integration/integration_controller.go

@@ -217,6 +218,15 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 		return err
 	}

+	// Watch ServiceBindings created
+	err = c.Watch(&source.Kind{Type: &sb.ServiceBinding{}}, &handler.EnqueueRequestForOwner{


I think this may cause issues in case the ServiceBinding resource is not present in the cluster (or not yet present when the Camel K operator is installed). We have a similar issue with Knative where we ended up watching the ReplicaSet instead of specific Knative resources (right @astefanutti ?). Is there a child standard resource that we can watch?

Ok, I see you're already targeting this. There's also a safer way to look if you can watch a resource by using these APIs:

camel-k/pkg/util/kubernetes/permission.go

Line 33 in 51a5c23

func CheckPermission(ctx context.Context, client client.Client, group, resource, namespace, name, verb string) (bool, error) {

Also adding a watch on sub-resources (if possible) that change can help in case the servicebinding operator is installed after the camel k operator is started.

Thanks for reviewing! There is the status subresource if that helps ?

No, I was referring to a child-resource. Like if you know that servicebinding always syncs a secret with some labels when updating the binding resource, then listening for changes in the secrets (all or with filter) triggers the reschedule of the integration, so it proceeds.
Secrets are an example of "watchable" resource being in core v1.

That would be useful only in cases where ServiceBinding is installed after camel k.

Ah right no not by labels, here's a sample secret:

apiVersion: v1 data: ... kind: Secret metadata: name: sample-service-binding-request namespace: service-binding ownerReferences: - apiVersion: operators.coreos.com/v1alpha1 controller: true kind: ServiceBinding name: sample-service-binding-request selfLink: /api/v1/namespaces/service-binding/secrets/sample-service-binding-request type: Opaque

We could check in the owner references and work our way up to the integration name by checking if it exists but it may be simpler to file a request on the Service Binding Operator side to copy labels from the ServiceBinding to the generated Secret which seems at first glance like a reasonable request...

Tracked with redhat-developer/service-binding-operator#852

nicolaferraro · 2021-01-28T09:20:30Z

pkg/controller/integration/platform_setup.go

@@ -46,7 +46,8 @@ func (action *platformSetupAction) Name() string {
 // CanHandle tells whether this action can handle the integration
 func (action *platformSetupAction) CanHandle(integration *v1.Integration) bool {
 	return integration.Status.Phase == v1.IntegrationPhaseNone ||
-		integration.Status.Phase == v1.IntegrationPhaseWaitingForPlatform
+		integration.Status.Phase == v1.IntegrationPhaseWaitingForPlatform ||
+		integration.Status.Phase == v1.IntegrationPhaseWaitingForServiceBindingCollectionReady


Is this needed? I think it does not do anything related.

I added the IntegrationPhaseWaitingForServiceBindingCollectionReady phase to the platform_setup action but I could create a new one and add it there? It was convenient because it's the first one called and setting up the servicebinding should be done as early as possible. Also this Action does not modify the IntegrationPhase which allowed me to set it in the trait.

Yes, sounds good

Done, thanks

nicolaferraro · 2021-01-28T09:21:38Z

pkg/trait/service_binding.go

+// IsAllowedInProfile overrides default
+func (t *serviceBindingTrait) IsAllowedInProfile(profile v1.TraitProfile) bool {
+	return profile == v1.TraitProfileKubernetes ||
+		profile == v1.TraitProfileOpenShift


Is there a reason why the 3rd profile is excluded (Knative)?

No, removed

nicolaferraro

Everything sounds good! Can you just check the conflicts?

johnpoth · 2021-02-08T13:23:44Z

Rebased, thanks !

nicolaferraro · 2021-02-09T11:31:10Z

Ok, conflicts again and I've tried to use this nice UI tool that github provides but made things worse XD

Can you check again (and delete my commit).. We'll merge before waiting the CI this time

…lled (apache#1445)

…ow ServiceBinding trait on all profiles (apache#1445)

johnpoth · 2021-02-09T12:25:47Z

@nicolaferraro done! Thanks

johnpoth added the kind/feature New feature or request label Jan 27, 2021

nicolaferraro requested changes Jan 28, 2021

View reviewed changes

nicolaferraro approved these changes Feb 8, 2021

View reviewed changes

johnpoth force-pushed the service-binding branch from 64a6bcb to 619bada Compare February 8, 2021 13:22

johnpoth force-pushed the service-binding branch from 619bada to c58ca42 Compare February 8, 2021 13:54

johnpoth added 6 commits February 9, 2021 13:11

feat: Add ServiceBinding trait (apache#1445)

b99ec29

Do not watch ServiceBindings if Service Binding Operator is not insta…

6622477

…lled (apache#1445)

fix: install ServiceBinding roles for OLM, HELM and kamel install

05b422e

fix: use checkPermission to check if ServiceBinding is installed, all…

2fde766

…ow ServiceBinding trait on all profiles (apache#1445)

fix: add new action that waits for bindings to be ready (apache#1445)

c46486c

Update service-binding RBAC resources to new layout (apache#1445)

cd8a603

johnpoth force-pushed the service-binding branch from c3868f0 to cd8a603 Compare February 9, 2021 12:24

nicolaferraro merged commit d36f4d0 into apache:master Feb 9, 2021

johnpoth deleted the service-binding branch February 9, 2021 12:45

johnpoth mentioned this pull request Feb 9, 2021

Take a look at service binding #1445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ServiceBinding trait (#1445) #1952

feat: Add ServiceBinding trait (#1445) #1952

johnpoth commented Jan 27, 2021

nicolaferraro left a comment

nicolaferraro Jan 28, 2021

nicolaferraro Jan 28, 2021 •

edited

Loading

johnpoth Jan 28, 2021

nicolaferraro Jan 28, 2021

nicolaferraro Jan 28, 2021

johnpoth Jan 29, 2021

johnpoth Feb 5, 2021

nicolaferraro Jan 28, 2021

johnpoth Jan 29, 2021 •

edited

Loading

nicolaferraro Feb 3, 2021

johnpoth Feb 5, 2021

nicolaferraro Jan 28, 2021

johnpoth Jan 29, 2021

nicolaferraro left a comment

johnpoth commented Feb 8, 2021

nicolaferraro commented Feb 9, 2021

johnpoth commented Feb 9, 2021

feat: Add ServiceBinding trait (#1445) #1952

feat: Add ServiceBinding trait (#1445) #1952

Conversation

johnpoth commented Jan 27, 2021

nicolaferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaferraro Jan 28, 2021 • edited Loading

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

johnpoth Jan 29, 2021 • edited Loading

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

nicolaferraro left a comment

Choose a reason for hiding this comment

johnpoth commented Feb 8, 2021

nicolaferraro commented Feb 9, 2021

johnpoth commented Feb 9, 2021

nicolaferraro Jan 28, 2021 •

edited

Loading

johnpoth Jan 29, 2021 •

edited

Loading