servicebinding / website

Repository hosting the website files (guides, etc)
https://servicebinding.io
Apache License 2.0
0 stars 4 forks source link

Type of jsonpath query results in workload resource mapping isn't well-defined #14

Open sadlerap opened 2 years ago

sadlerap commented 2 years ago

Something I ran into with regards to workload resource mapping: I was working with mapping a CronJob, and I was trying to determine if we should fail when we have a path query of .spec.jobTemplate.spec.template.spec.containers (rather than .spec.jobTemplate.spec.template.spec.containers[*], which is based off of our default). The difference between these two is that the first gives an array of container objects, while the second returns the same results, albeit not boxed in an array (i.e. multiple return values).

Should implementations support both of these queries as equivalent alternatives, or is one of these more correct than the other?
The default path query uses .spec.template.spec.containers[*], which inclines me toward preferring the latter type of query. That said, I don't think there's anything preventing implementations from supporting both styles (aside from maintenance burden). The specification doesn't say anything about this, however.


Filing this here because this is largely related to implementation work, and I think this should be mentioned in the implementation guide. That said, it may require spec modifications.

scothis commented 2 years ago

Should implementations support both of these queries as equivalent alternatives, or is one of these more correct than the other?

I had the same thought while working on the proof of concept that lead to the current definition in the spec. The key difference is that we're looking for places where a container-like structure exists in the workload, not an array of containers. The [*] bit on the end of the JSONPath expression converts a single array of containers into a set of individual containers. For example, the RuntimeComponent resource has a container-like structure that is a single struct and not a member of an array.

The current spec is guiding users to the most technically precise answer. That said, it may be worth relaxing this to make it more user friendly. Within the current implementation, I believe we can sniff the returned nodes to determine if they are objects or arrays and flatten the array. I'm not sure how much of a burden this would impose on other implementations.

At a technical level:

sadlerap commented 2 years ago

The current spec is guiding users to the most technically precise answer.

I didn't quite get that understanding from the spec. AIUI, it implies via the defaults what kind of results it's looking for, but I didn't see it explicitly spelled out anywhere. I'd like to try to avoid "implied" behavior like this in the spec if possible; I'd rather not end up with implementations disagreeing on what is and is not spec compliant.

That said, it may be worth relaxing this to make it more user friendly. Within the current implementation, I believe we can sniff the returned nodes to determine if they are objects or arrays and flatten the array. I'm not sure how much of a burden this would impose on other implementations.

This kind of sniffing is what I'm doing in our implementation for now. It does make things more user friendly in my opinion. That said, it would be nice to specify precisely what is and is not going to be accepted; I'd like to avoid disagreeing implementations.

scothis commented 2 years ago

The spec says:

A MappingContainer object MUST define a path entry which is a string containing a JSONPath that references container like locations in the target resource.

I don't read this as talking about arrays of "containers", but I agree it's not clear enough in the other direction. For example, the JSON Paths for volumes/volumeMounts/env are all clearly defined as being for arrays of a specific type.