Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specifications for Dapr Binding Implementation #68

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SoTrx
Copy link

@SoTrx SoTrx commented Oct 15, 2024

Hey,

Here are the proposed specifications for the Dapr Binding implementation.

@lakshmimsft
Copy link
Contributor

Hello @SoTrx. Big thanks for the design doc PR. I've given some initial comments mainly requesting more details in some areas but looking good overall. Thanks a lot!

Copy link
Contributor

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @SoTrx - just wanted to drop by and say thanks for doing this. I'll be traveling during the discussion on Monday. I think the template is well filled out 👍 and will lead to the right discussion.

Copy link
Contributor

@kachawla kachawla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SoTrx, this is a well written doc! Main area that I would like to discuss is the proposal to support manual provisioning instead of recipe based provisioning, pros and cons that led to this decision.

resources/2024-10-dapr-bindings.md Outdated Show resolved Hide resolved
resources/2024-10-dapr-bindings.md Outdated Show resolved Hide resolved
resources/2024-10-dapr-bindings.md Show resolved Hide resolved
resources/2024-10-dapr-bindings.md Outdated Show resolved Hide resolved
resources/2024-10-dapr-bindings.md Outdated Show resolved Hide resolved
resources/2024-10-dapr-bindings.md Show resolved Hide resolved
@ytimocin
Copy link
Contributor

Thanks @SoTrx, this is a well written doc! Main area that I would like to discuss is the proposal to support manual provisioning instead of recipe based provisioning, pros and cons that led to this decision.

Just to better understand @SoTrx, we are going to be supporting both types of provisioning by default, right?

@SoTrx
Copy link
Author

SoTrx commented Oct 22, 2024

Thanks @SoTrx, this is a well written doc! Main area that I would like to discuss is the proposal to support manual provisioning instead of recipe based provisioning, pros and cons that led to this decision.

Just to better understand @SoTrx, we are going to be supporting both types of provisioning by default, right?

In my view, recipes aren't necessary for bindings, as they only create the CRD and could lead to a two-step process for creating a single resource.

However, since this indeed contradicts Radius' design policy, how about introducing "common usage binding recipes"?

For instance, we could define an "object-storage binding recipe" that:

  • In local development: creates both the binding CRD and the storage backend (e.g., MinIO). (Sample binding)
  • In production (Azure): only creates the binding CRD, with a parameter in the recipe to provide the storage account details. (Sample binding)

This approach aligns with Radius' philosophy, is convenient for developers, and retains the core concept of bindings.

To maximize convenience for the user, these recipes will be designed with the following considerations:

- **Local Development**: The recipe will create both the binding CRD and the "external resource".
- **Production**: The recipe will create only the binding CRD, with the user providing the necessary information for connecting to the external resource.
Copy link
Contributor

@rynowak rynowak Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid usecase (pre-existing resource), but it's not the main one.

We'd usually think about the platform or infrastructure team as being the one to choose the recipes for production usage.

We don't need to impose constraints.

**typespec/Applications.Dapr/bindings.tsp**
```TypeSpec
model DaprBindingResource
is TrackedResourceRequired<DaprBindingProperties, "DaprBindings"> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type name is bindings then this should be TrackedResourceRequired<DaprBindingProperties, "bindings">


- (out-of-scope) Managing **external resources** used by Dapr Bindings. As we do not have management rights over the external resources, we will not be able to create, update, or delete them. The user will need to create the external resource and provide the necessary information to the Dapr Binding.

### User scenarios (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both user scenarios, I recommend elaborating on the user ultimately leveraging Dapr Bindings to accomplish the abstraction, and then they are wanting to make use of these Dapr Bindings in Radius applications. e.g.

As an operator using Radius, I want the application I manage to interface with other applications in my organization without exposing their implementation details to the developers by leveraging Dapr Bindings. Thus, I would like to define and use Dapr Bindings as a resource type within my Radius applications.


Functional tests proposed:
- Using a binding in a sample application (the direction, input/output, of the binding is not important for this test). The external system can be something simple like Redis or a [CRON binding](https://docs.dapr.io/reference/components-reference/supported-bindings/cron/) that doesn't require an external system.
- Using a binding with secret-store indirection, which will require creating and using a secret store in the binding.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 These are a good set of functional tests. 👍 also to avoiding external dependencies for testing where possible.

## Design Review Notes

Round 1:
- In the first iteration, recipes were considered out of scope, as they would result in a two-step process to create a single CRD (the Dapr Binding component). This was reconsidered, as not supporting recipes for a resource contradicts the Radius design philosophy. Therefore, common usage recipes were added to the design.
Copy link
Contributor

@rynowak rynowak Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand this - apologies, I was out of the office when this was discussed. We have existing recipes for Dapr components that create both the infrastructure and the Dapr Component resource, so I'm not aware of any issues. LMK if I'm missing anything!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

There is no issue with the creation process itself. You can create both the Binding CRD and the infrastructure just as you would with any other Dapr Building Block. Users can (and likely will) do this with their own configurations, and we certainly shouldn’t try to restrict that flexibility.

My point is only relevant to the initial binding recipes we plan to provide to users. Here’s some context: the Bindings Building Block is unique in Dapr. It predates most other building blocks and originally served as a way to accomplish tasks that did not yet have dedicated building blocks. This is why some bindings overlap with other blocks, such as Azure Storage Queue.

Bindings are generally basic, often supporting only CRUD-like operations. Additionally, Bindings is the only building block without a unified feature set across multiple backends—nearly every binding is different, and few are interchangeable.

Today, bindings are intended for interacting with external resources, as they are designed for basic interactions. For internal, app-scoped backing services, there is usually a better building block available.

While this approach doesn’t affect how Radius implements Bindings, it does impact the default recipes we include in radius/recipes. If we consider bindings best suited for external resources, then a binding recipe should only contain the Dapr Component CRD. This is why I initially opposed creating binding recipes, as it would involve a two-step process for what is effectively a single CRD creation.

In the first meeting, however, a very good point was raised: from Radius's perspective, it is difficult to justify not having any recipes for a resource.

So, my revised proposal is to create default recipes with this "external resource" perspective in mind. These recipes would set up the backing service in local-dev for convenience, but in production, they would have as input the credentials to an existing backing service. This approach isn’t meant to restrict users but rather as a way to nudge users in the right direction from the start. Of course, users can create custom recipes to override this behavior as they see fit.

I'm not entirely sure if I explained this clearly or if it make sense, so please don’t hesitate to challenge me or ask for clarification on any part of this! And if this direction makes sense, I’m happy to revise the design doc to reflect it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my revised proposal is to create default recipes with this "external resource" perspective in mind. These recipes would set up the backing service in local-dev for convenience, but in production, they would have as input the credentials to an existing backing service. This approach isn’t meant to restrict users but rather as a way to nudge users in the right direction from the start. Of course, users can create custom recipes to override this behavior as they see fit.

Hey @SoTrx - I appreciate the response 👍

I think the part where I'm stuck is why we need recipes for the "pre-existing" case. This is what resourceProvisioning: 'manual' is for.... when you want to configure the metadata directly.

I'm happy to continue this discussion when we start looking at recipe examples since it's not really going to change the design of the bindings resource type.

Copy link
Contributor

@ytimocin ytimocin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than a few nits, LGTM. Thanks!

Copy link
Contributor

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for the updates!

@rynowak
Copy link
Contributor

rynowak commented Nov 9, 2024

Hi @SoTrx - can you take a look at the spellcheck errors? https://github.com/radius-project/design-notes/actions/runs/11671732590/job/32742154388?pr=68

For any false-positives reported by the spellcheck you can add them here: https://github.com/radius-project/design-notes/blob/main/.github/config/en-custom.txt

Or if they are technical terms I like to use backquotes to turn them into code. like this!

After that I think this is ready to merge.

@SoTrx
Copy link
Author

SoTrx commented Nov 21, 2024

Hi @SoTrx - can you take a look at the spellcheck errors? https://github.com/radius-project/design-notes/actions/runs/11671732590/job/32742154388?pr=68

For any false-positives reported by the spellcheck you can add them here: https://github.com/radius-project/design-notes/blob/main/.github/config/en-custom.txt

Or if they are technical terms I like to use backquotes to turn them into code. like this!

After that I think this is ready to merge.

Hey Ryan,
Sorry, I haven’t been able to work on this lately. Could you rerun the CI? I can’t view the latest spellcheck summary.
Thanks!

@rynowak
Copy link
Contributor

rynowak commented Nov 26, 2024

@SoTrx - launched the workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants