make contract server-side validations as good as the client side ones in the CLI

[migmartri]

When creating a contract through the API, if it contains a typo on the actual fields of the json, it throws an error from the buf validation, we should return a better error as in the CLI:

$ chainloop wf contract create --name "testing" -f 'testing/scratch_8.yml'
ERR proto: (line 1:472): unknown field "type:"

Interesting enough, using grpcurl produces the same error:

$ grpcurl -plaintext -d "$(cat 'testing/scratch_8.yml')" -H "Authorization: Bearer $TOKEN" localhost:9000 controlplane.v1.WorkflowContractService/Create
Error invoking method "controlplane.v1.WorkflowContractService/Create": error getting request data: message type workflowcontract.v1.CraftingSchema.Material has no known field named type:

[migmartri]

{
  "schemaVersion": "v1",
  "materials": [
    {
      "type": "CONTAINER_IMAGE",
      "name": "skynet-control-plane",
      "output": true,
      "annotations": [
        {
          "name": "component",
          "value": "control-plane"
        },
        {
          "name": "asset"
        }
      ]
    },
    { "type:": "ARTIFACT", "name": "rootfs" },
  ]
}

[javirln]

Doing a research on the topic I've found the following:

• Both the grpcurl and the CLI start from the same state, need to convert from a json to a proto structure. Such conversation is the one that is raising the error unknown field "type:".
• Server side validation using a grpc client work in a different way. From typescript, the value is being transformed to whatever the proto definition says and ignores unknown types. That ignoring of unknown types mean that when passing a type: on that material the actual type would be of type type: MATERIAL_TYPE_UNSPECIFIED which is the default value of the enum.
• protovalidate has a nice feature where you can skip on the middleware the validation of certain messages doing the following change, which will leave the validation to the developer in a later stage.

diff --git a/app/controlplane/internal/server/grpc.go b/app/controlplane/internal/server/grpc.go
index 5754ff3..ec4a610 100644
--- a/app/controlplane/internal/server/grpc.go
+++ b/app/controlplane/internal/server/grpc.go
@@ -97,7 +97,11 @@ func NewGRPCServer(opts *Opts) (*grpc.Server, error) {
 		grpc.Middleware(craftMiddleware(opts)...),
 		grpc.UnaryInterceptor(
 			grpc_prometheus.UnaryServerInterceptor,
-			protovalidateMiddleware.UnaryServerInterceptor(opts.Validator),
+			protovalidateMiddleware.UnaryServerInterceptor(opts.Validator,
+				// Any message that is registered here will not be validated, therefore, validation
+				// must happen somewhere else, proceed with caution
+				protovalidateMiddleware.WithIgnoreMessages((&v1.WorkflowContractServiceCreateRequest{}).ProtoReflect().Type()),
+			),
 		),
 	}
 

What this means is that, when reaching the server, the validations are run thanks to protovalidate and raise validation error: - v1.materials[4].type: value must not be in list [0] [enum.not_in] and this is because the incoming proto although well formed is not valid upon our constraints.

We cannot perform the same operation as the CLI does under the hood since what is received on the server is correct although not valid so going from proto => json => proto won't work because the type would still be type: MATERIAL_TYPE_UNSPECIFIED, if we later run the validation, then we will fail.

The solution then, would be on the approach of improving the error message. Unfortunately the buf validation library although wide, does not provide much more configuration except the cel option:

Something like this will perform in the same way as the enum validation we have in place. From here:

    MaterialType type = 1 [
      (buf.validate.field).enum = {
        not_in: [0]
      },
      (buf.validate.field).enum.defined_only = true
    ];

To here:

    MaterialType type = 1 [(buf.validate.field).cel = {
      id: "type"
      message: "Type must be one of the allowed values"
      expression: "this != 0 && this in [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]"
    }];

As you can see one of the drawback is, that we need to keep track of the list of values.

To sum up, we need to find a way of improving the general return of errors in the API level.

[danlishka]

what are the next steps on this then?

[migmartri]

I'd like to double check the possibility of improving validation error messages server side. It's very discouraging that at first this is not trivial. So next step is to look into how hard is to have custom error messages applied to the proto validations.