Improve render status for porch

ChristopherFry commented 2 years ago

Creating an umbrella issue to track requested improvements and issues with the Porch render status.

Porch does not set Exit Code field on render status

When kpt functions are executed through Porch, the ExitCode field in renderStatus.result and renderStatus.result.Items[] are always set to zero regardless if a function errors.

This can be reproduced by adding set-namespace mutator to the Kptfile, and setting configPath input to an non-ConfigMap and non-SetNamespace resource.

In Porch, the following is retuned:

kind: PackageRevisionResources
apiVersion: porch.kpt.dev/v1alpha1
...
status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v0.4.1
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results:
        - message: unknown functionConfig Kind=Kptfile ApiVersion=kpt.dev/v1, expect
            `SetNamespace` or `ConfigMap`
          severity: error
    error: "fn.render: pkg .:\n\tpkg.render:\n\tpipeline.run: error: function failure"

whereas with kpt fn render the exit code field is set:

apiVersion: kpt.dev/v1
kind: FunctionResultList
metadata:
  name: fnresults
exitCode: 1
items:
  - image: gcr.io/kpt-fn/set-namespace:v0.4.1
    stderr: 'failed to evaluate function: error: function failure'
    exitCode: 1
    results:
      - message: unknown functionConfig Kind=Kptfile ApiVersion=kpt.dev/v1, expect `SetNamespace` or `ConfigMap`
        severity: error

The impact of this is that clients of Porch will not be able to use the ErrorCode field to determine if a function failed.

Porch does not set Stderr field on render status

Similar to the exit code field, Porch also does not set the Stderr field.

This can be reproduced by adding an invalid function to the Kptfile.

In Porch, the following is returned:

status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v5.0.0
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results:
    error: "fn.render: pkg .:\n\tpkg.render:\n\tpipeline.run: func eval \"gcr.io/kpt-fn/set-namespace:v5.0.0\"
      failed: rpc error: code = Unknown desc = unable to get the grpc client to the
      pod for gcr.io/kpt-fn/set-namespace:v5.0.0: unable to get the entrypoint for
      gcr.io/kpt-fn/set-namespace:v5.0.0: GET https://gcr.io/v2/kpt-fn/set-namespace/manifests/v5.0.0:
      MANIFEST_UNKNOWN: Failed to fetch \"v5.0.0\" from request \"/v2/kpt-fn/set-namespace/manifests/v5.0.0\"."

Whereas with kpt fn render the stderr field is set:

apiVersion: kpt.dev/v1
kind: FunctionResultList
metadata:
  name: fnresults
exitCode: 1
items:
  - image: gcr.io/kpt-fn/set-namespace:v5.0.0
    stderr: |-
      docker: Error response from daemon: manifest for gcr.io/kpt-fn/set-namespace:v5.0.0 not found: manifest unknown: Failed to fetch "v5.0.0" from request "/v2/kpt-fn/set-namespace/manifests/v5.0.0".
      See 'docker run --help'.
    exitCode: 125

The impact of this is that clients of Porch cannot always iterate through the Items list to determine which function failed. Fortunately the error is returned on the top level error field however it's not associated with a function.

Exclude the Exec Path field from render results

Since functions are executed server side in Porch, this field will likely not be valuable to Porch clients and may leak internal implementation details.

status:
  renderStatus:
    result:
      kind: FunctionResultList
      apiVersion: kpt.dev/v1
      metadata:
        name: fnresults
        creationTimestamp:
      ExitCode: 0
      Items:
      - Image: gcr.io/kpt-fn/set-namespace:v5.0.0
        ExecPath: ''
        Stderr: ''
        ExitCode: 0
        Results: ...

Inconsistent field casing

The casing of the fields in FunctionResultList is inconsistent with Porch and kpt fn render. The fields in kpt fn render are camelCase whereas the same fields in Porch are mostly PascalCase.

This can be seen in the examples above.

Requesting FunctionResultList always includes all functions, including those not executed

This is a feature request for the FunctionResultList (for both Porch and kpt) to include all functions, including any functions that were not executed and/or skipped. Today you only know if a function is skipped by comparing the functions in the FunctionResultList to those in the Kptfile.

Requesting rendering does not stop if a validation function fails

This is a feature request to execute all Kptfile validation functions, even if one of the validation functions fails. An advantage of this is that this will allow us to show the user all validation failures at one time, not just the first validation failure.

ChristopherFry commented 2 years ago

cc @droot

ChristopherFry commented 2 years ago

Inconsistent field casing The casing of the fields in FunctionResultList is inconsistent with Porch and kpt fn render. The fields in kpt fn render are camelCase whereas the same fields in Porch are mostly PascalCase.

This is resolved with #3638.

kptdev / kpt