wasmCloud / interfaces

Deprecated: wasmCloud API interfaces: smithy IDLs and shared libraries
https://wasmcloud.github.io/interfaces/
Apache License 2.0
34 stars 22 forks source link

(sqldb) pass args to provider for interpolation #41

Closed ricochet closed 1 year ago

ricochet commented 2 years ago

Change from Fetch to Query to better align with SQL terminology.

Add a "args" input parameter so that the SQL capability provider can perform interpolation and avoid SQL injection issues in the actors.

I bumped the version for the sqldb interface. Are there other steps I need to take?

ricochet commented 2 years ago

Traditionally the database choice is done by the configuration values inside a link definition. This way, the actor doesn't need to know the details of what it's connecting to or how it's connecting to it. Are you imagining a use case where the actor specifies a database and there are database selectors in the linkdef values? I worry about potential conflicts where if an actor picks a database and the actor has linkdefs changed in different environments and potentially doesn't have access to that database.

I could be reading this wrong, but I worry that putting a database selector in the actor's code could inhibit some of the portability we have in the runtime

Since the term database is overloaded, I'm going to define a few things first. Today's link definition includes the database protocol, database credentials, host, port, and application database. For example:

"uri": "postgresql://$APP_DB_USER:$app_pass@$APP_DB_HOST:$DB_PORT/$APP_DB_NAME"

The database connection or DB_URI in this example is: "postgresql://$APP_DB_USER:$app_pass@$APP_DB_HOST:$DB_PORT".

I 100% agree the database connection should continue to be defined in the link definition. What we're discussing next is this piece, "$APP_DB_NAME".

Question: Can an actor connect to only one database in a cluster?

If the database provider is a clustered database provider, then actors may want the flexibility of attaching to multiple databases within the cluster. Contrived case: "HR management actor for removing an employee from employees db as well as removes them from the benefits database".

Generally I don't expect platforms to strictly enforce software design standards like "1:1 of actor:resource:database", but this is lofty stuff and I don't have a big opinion on that front. I was thinking about this from a perf perspective that I'll outline below.

If an actor may connect to more than one database, how is this specified? It can no longer be a part of the DB_URI although the DB_URI may have a default database. Since the connection can be re-used for either application database in the cluster, we would still only desire a single URI in the link definition. An added benefit of not restricting the database connection to a single database is that the connection pool can be more efficient.

Today the connection pool is pooled by the actor_id. A different implementation with the same effect is using "DB_URI+App Database" as the key to the connection pool. Alternatively, we could pool across actors. We can't today because we don't know which database a given Query or Execute function call is targeting. An alternative to what I have in the PR is to parse the uri in the config for the "APP_DB_NAME" then use that to set the used database. I still have the question about whether or not an actor should be able to query more than one database.

For a single application made up of many actors, it is very likely that they will all connect to the same database cluster. Say we have a connection limit of 2 and have 3 actors. All actors connect to mydb.com:1234 with the same set of credentials (a little gross but super common). The way the pool works today, is that it will close and re-open a new connection if the calling actor is not one of the 2 most recent actors. If pooling is across actors and we know the database used by a given query, then we can continue to re-use either of the available connections. At a massive scale, this can be a very powerful differentiator.

autodidaddict commented 2 years ago

the way wasmCloud approaches this problem is with the concept of link names. If an actor has need for two logically different implementations of a wasmcloud:sqldb provider, then the actor can use logical names (link names) to differentiate.

So, for example, let's say this actor needs to talk to the employees and benefits databases. The actor should never know the real "physical" names of those databases because that would violate the rule against bleeding non-functional requirements into actor space. So, what we would do is create two instances of the wasmcloud:sqldb capability provider:

The code inside the actor would then logically distinguish between target "SqlDbProvider" instances by the use of the logical name. If we wanted to switch the employees database to be on a different SQL instance as we scale, then the actor would never be the wiser. Forcing the actor to assume that employees and benefits will always have a shared set of credentials and connection host/port limits the power of that actor and the options/flexibility that operations has.

So, tl;dr, I would keep $app_db_name inside a link definition and recommend to someone building an app like this that if they need two logically separate data stores that implement the wasmcloud:sqldb contract, they make use of the link name feature.

stevelr commented 2 years ago

I like that you made parameters also cbor values. I'm curious about the use cases for Ping: (1) do you expect that the same actors that use the query api will also do pings, or do see that as a separate health check function? If it's done by a separate health checker process, it might be better to go into a separate interface. (2) how would Ping work in the presence of connection pools? If a connection pool is allowed to let the number of idle connections drop to zero, would ping fail, or would it attempt to spin up a new connection?

ricochet commented 2 years ago

Are instances of the same capability provider with different link definitions aware of each other?

ricochet commented 2 years ago

For encoding of the parameters as cbor, this doesn't quite work as-is. I wanted to use a union but that isn't yet supported. With CBOR I need to know the schema for the query parameters to decode. One option is to add another field, ParametersSchema, that is an enumeration of the parameter types. Alternatively, we work on getting unions supported. Thoughts?

ricochet commented 2 years ago

I'm curious about the use cases for Ping: (1) do you expect that the same actors that use the query api will also do pings, or do see that as a separate health check function? If it's done by a separate health checker process, it might be better to go into a separate interface. (2) how would Ping work in the presence of connection pools? If a connection pool is allowed to let the number of idle connections drop to zero, would ping fail, or would it attempt to spin up a new connection?

In the wild, I have seen serverless functions for long running polling functions call a ping-like api. This is a close approximation to an actor. But that framework did not have a concept of health-checks. If wasmCloud is considering a general interface for health-checks or health-checks for certain providers, then the sqldb provider should implement that interface, and the underpinnings will essentially be a ping.

Whenever there are persistent connections over TCP, things get a little dicey with cloud providers. We've run into issues on one of the big-three where ports go stale but not closed. It's only after a query is run do we recognize there is an error. In that scenario, the ideal handling would be a retry of the query not something the actor itself should have to handle. For providers that are maintaining a connection pool of active or idle connections, I believe a health-check/keepalive system is necessary. (2) I expect ping to establish a new connection and close after a configured idle timeout.

stevelr commented 2 years ago

Alternatively, we work on getting unions supported. Thoughts?

I was hoping for something in the spirit of serde_json::Value, but for CBOR. You should be able to ask it for its type.

stevelr commented 2 years ago

Are instances of the same capability provider with different link definitions aware of each other?

No. A capability provider wouldn't even know about other instances of itself with the same link definition (on other hosts).

stevelr commented 2 years ago

I believe a health-check/keepalive system is necessary.

Those are two separate concepts. There already is a basic health check system that asks each actor and capability provider for its health status about every 30 seconds. It relies on the pingee being able to self-diagnose its health. A process that monitors these health check events can accumulate over time a global picture of what's working. That's something that the Lattice Monitor does.

As far as keep-alives for database connections, I would think that should be the job of the connection pool maintained by the capability provider - and should be invisible to the actor. The connection pool knows about the specifics of the db it is connected to, so it can do the necessary queries at the appropriate frequency to keep the connection alive.

brooksmtownsend commented 1 year ago

Closing this issue as it's gone a little stale