apache / incubator-kie-kogito-runtimes

Kogito Runtimes - Kogito is a cloud-native business automation technology for building cloud-ready business applications.
http://kogito.kie.org
Apache License 2.0
536 stars 206 forks source link

Dynamic OpenAPI configKey #3573

Closed fjtirado closed 2 months ago

fjtirado commented 3 months ago

Description

OpenAPI properties depends on configKey For example, the property for the URL is of the form quarkus.rest-client.<configKey>.url configKey is currently calculated at build time (there are different alternatives based on the name and content of the openapi spec file being parsed, by default configKey is the sanitized file name) The proposal is to allow dynamic calculation of configKey at runtime. This will allow appending extra information to the configKey calculated at build time. For example, you can have different url properties depending on a cluster name

quarkus.rest-client.<buildTimeConfigKey>.cluster1.url=<cluster1Address>
quarkus.rest-client.<buildTimeConfigKey>.cluster2.url=<cluster2Address>

In order SonataFlow to know which information should be used to calculate configKey, users should add a new property, called sonataflow.rest-client.dynamic-config-key, which value must be a jq expression which evaluation will return the value of the dynamic configKey

For the previous example, assuming the workflow input has a clusterName which value is cluster1 or cluster2, users must add this line to application.properties.

sonataflow.rest-client.dynamic-config-key=.clusterName

Implementation ideas

No response

rgolangh commented 2 months ago

There is a delicate detail here - that the context variable used in the jq expression must exist at the state where this invocation happens. it is very easy to override context variables by other states and loose them. So lets say the usage of this openapi action is several state down the workflow, there is a huge chance .clustername would just be overwritten.

How can we make sure this configkey selection strategy is easy to grasp, harder to ruin, easy to debug if needed ? can we analyze the states that overwrite the whole data context and warn? maybe at build time? but then again the config key is runtime mutable.

fjtirado commented 2 months ago

There is a delicate detail here - that the context variable used in the jq expression must exist at the state where this invocation happens. it is very easy to override context variables by other states and loose them. So lets say the usage of this openapi action is several state down the workflow, there is a huge chance .clustername would just be overwritten.

How can we make sure this configkey selection strategy is easy to grasp, harder to ruin, easy to debug if needed ? can we analyze the states that overwrite the whole data context and warn? maybe at build time? but then again the config key is runtime mutable.

Really good point!!! My first idea was that we should evaluate the expression at the moment the OpenAPI invocation is done. And indeed the possibility that the expression returns nothing do exist. After thinking about the alternatives, one possibility is evaluating also when the workflow is started. Therefore the algoritm will be, if the property quarkus......runtimeConfigKey is set, then it is evaluated against the input model when the workflow is started, and the calculated value, if existing, is stored as part of the process instance internal info (that is keep in DB). Then, when the openapi call happened, it is evaluated again. If there is a value, thats the value to be used in the call. If there is not value, then the value calculated when the workflows was started, if existing, will be used. If there is not value, then the build time config key will be used. If not matching property is found, then the URI in the spec will be used (and the call will very likely faild because wrong uri, we can add message to review the properties, naming them)

rgolangh commented 2 months ago

I'm trying to capture what you say:

problem with that is that this is a surprizing API because the users can't really know what value will be used if they for example don't send it. So a call to a workflow without a param $clustername, could lead to a some other default (buildtime) key to be used, where the user didn't expect, and even the workflow could even create resources on that (wrong) cluster.

But perhaps I didn't get your proposal properly. Please correct me if anything here is wrong.

Can we consider a more explicit API, either we have the value or fail? I would expect something like providing an argument to the openapi function that will specify the key:

functions:
  - name: createDeployment
    operation: kube.yaml#createDeployment
    restConfigKey: EXPRESSION

so now it is at least visible that EXPRESSION is resolved during invocation. Plus, add syntax to access the request params , or the one saved in the db(similar to how SECRETS is to application pros):

restConfigKey: 'REQUEST.clusterName'  

And whatever the resolution of the key is, that is what is used. otherwise failure.

Again, my point of view is, making the developer of the workflow easy to understand and use those nice configuration items, but with limitation that will still prevent mistake, and will be easier to debug.

fjtirado commented 2 months ago

@rgolangh I think your prposal is fine. However, as you know, we cannot add keys to functions directly, we wil have use metadata for that, therefore it will look like

functions: 
   - name: createDeployment
     operation: kube.yaml#createDeployment
     metadata: 
         configKey: expression

However, this just change the place where the config key expression is defined. I was proposing to define it as property, now is being defined as metadata in the process definition. And we are deciding that it will be evaluated upon invocation (I agree that is the most obvious place) But the issue of keeping the cluster name in the workflow model will be still there (specially if the cluster name property is an input param). Also, we are deciding that if the expresion returns null, we fail rather than default.

rgolangh commented 2 months ago

I don't know if a key in metadata is better than a property.

To rephrase my other suggestions in a clearer way:

fjtirado commented 2 months ago

Ok, key is there or is not there, if not there, failure, not default We need to store input parameters (which we are not doing right now) in order to support $REQUEST, which I would rename as $INPUT ( a workflow can also be started through an event)

fjtirado commented 2 months ago

Metadata has the advantage (over the property) that is more clearly associated with the function, but in the property you can play with global expression and function scope expression by prefixing so you can have ..... rest.config-key = this is global and ... rest.<function-name>.config-key= this affects this function name only I do not have a strong preference

fjtirado commented 2 months ago

I was playing with the code and we can achieve global vs function scope with metatada also (just adding the metadata in the workflow or in the function)

fjtirado commented 2 months ago

With this PR, using this snippet

{
      "name": "subtraction",
      "operation": "specs/subtraction.yaml#doOperation",
      "metadata": {
         "configKey" : ".pepe"
      }
    }

we get as response


{
    "failedNodeId": "7",
    "id": "1d4d610e-5ad3-4844-ad57-42670669eb39",
    "message": "java.lang.IllegalArgumentException - Expression .pepe returns null or empty value"
}
fjtirado commented 2 months ago

@rgolangh Opened https://github.com/apache/incubator-kie-kogito-runtimes/issues/3590 for support $INPUT, rest of discussed funcitionality (using metadata rather than properties) was implemented with this issue

rgolangh commented 1 month ago

@rgolangh Opened #3590 for support $INPUT, rest of discussed funcitionality (using metadata rather than properties) was implemented with this issue

thank you