-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSONPath expressions in Application Resource Mapping and setting values #177
Comments
@baijum have you looked at https://pkg.go.dev/k8s.io/client-go/util/jsonpath#JSONPath.FindResults |
That function returns a slice of results. Should the implementation set value for each of the results? |
JSON Path can return multiple matching nodes for a query, the This raises an interesting question as what to do when there is an asymmetry between env and volumeMounts. The path of the volume mount needs to be based on the value of the |
IMHO, JSONPath is not the right tool for specifying locations in a data structure - it does great job of filtering the values but it does not provide a link between the matched value and its location in the data structure. Hence, updating the value and reflecting that update on the original structure is not something what JSONPath speaks about. It is left to an implementation to try to support it or not. When there is no match, the result is an empty list - Again, this does not give us any clue where relevant values should be injected if the data structure does not hold them. In the current spec, Application Resource Mapping defines I would propose that we replace JSONPaths with JSON Pointers - it a well known standard for locating data in JSON structures (e.g. among others |
As I understand, there are two primary use cases for Application Resource Mapping.
In both these cases, I don't see a requirement to support an array. BTW, the second example given for cronjob in the spec is not very realistic, because the previous example has a better solution. In the intermediate resource scenario, repeating the same data inside a nested data structure is not required. I think dot-separated values of strings would be sufficient to handle the current use cases. |
In order to support something like
The structure can have any other fields, but if it has these, we know what to inject and where. The location to that structure can be specified via a new field: apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: # string
generation: # int64, defined by the Kubernetes control plane
...
spec:
versions: # []Version
- version: # string
podSpecable: # jsonpointer or dot separated path The expression pointing to the location of PodSpecable resource do not need to contain any wildcards, and with that we cover already a huge number of cases. IMHO, it is very unlikely to have Hence, we can move from the need to support JSONPath, and use simpler standards, e.g. JSONPointer |
@nebhale I see that the term type PodSpecable struct {
InitContainers []corev1.Container `json:"initContainers,omitempty"`
Containers []corev1.Container `json:"containers,omitempty"`
Volumes []corev1.Volume `json:"volumes,omitempty"`
} Everything is optional. The rest of the fields in the found structure are not of interest for the operator. For a RuntimeComponent CR like: apiVersion: app.stacks/v1beta1
kind: RuntimeComponent
metadata:
name: my-app
spec:
applicationImage: quay.io/my-repo/my-app:1.0
service:
type: ClusterIP
port: 9080
expose: true
storage:
size: 2Gi
mountPath: "/logs"
apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: runtimecomponents.app.stacks
...
spec:
versions: # []Version
- version: '*'
podSpecable: .spec And then it is easy to create This works well for apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: cronjobs.batch
spec:
versions:
- version: "*"
podSpecable: .spec.jobTemplate.spec.template.spec |
How do you bind a service to the container with the image |
So, it seems that type PodSpecable struct {
InitContainers []corev1.Container `json:"initContainers,omitempty"`
Containers []corev1.Container `json:"containers,omitempty"`
Volumes []corev1.Volume `json:"volumes,omitempty"`
VolumeMounts [][]corev1.VolumeMount
Env []corev1.EnvVar
.
.
} so in order to bind, the operator locates the structure that should be modified, start looking what fields are available and based on that adds, in this case an entry under |
I agree that the current spec'd behavior is under defined and has issues, but I'm not convinced we need to take steps as dramatic as discussed here. Since the The essential capabilities we need to bind a service into an application workload are:
If we have access to a We need the flexibility to find container-esque shapes wherever they may be in the spec; JSONPath is a good fit. We also need the reliability to know within a found container-esque object a firm reference to the env and volumeMounts (JSON Pointer). While I'm not crazy about requiring two distinct grammars, we can explore a limited subset of JSONPath that gives us a JSON Pointer like experiance. apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata: # metav1.ObjectMeta
...
spec:
versions:
- version: # string
containers:
- path: # string(JSONPath)
name: # string(JSON Pointer), optional
env: # string(JSON Pointer), defaults to "/env"
volumeMounts: # string(JSON Pointer), defaults to "/volumeMounts"
volumes: # string(JSON Pointer) For the apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: runtimecomponents.app.stacks
spec:
versions:
- version: *
containers:
- path: .spec
- path: .spec.initContainers[*]
name: /name
- path: .spec.sidecarContainers[*]
name: /name
volumes: /spec/volumes What's interesting about RuntimeComponent is there are two obvious arrays of containers, but there's also a single implicit container in the root. This approach is flexible enough to capture all three. A traditional PodSpecable resource like apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: deployments.apps
spec:
versions:
- version: *
containers:
- path: .spec.template.spec.containers[*]
name: /name
- path: .spec.template.spec.initContainers[*]
name: /name
volumes: /spec/template/spec/volumes A non-PodSpecable resource like apiVersion: service.binding/v1alpha2
kind: ClusterApplicationResourceMapping
metadata:
name: cronjobs.batch
spec:
versions:
- version: *
containers:
- path: .spec.jobTemplate.spec.template.spec.containers[*]
name: /name
- path: .spec.jobTemplate.spec.template.spec.initContainers[*]
name: /name
volumes: /spec/template/spec/volumes |
This is not a real
With that we assume that all application CR semantic is the same as
JSONPath is for querying data, but not for specifying location. IMHO, containers/pods are all to be found in the same part of the resource, so no need to use the wildcard operator, and thus we really do not need to use JSONPath. For example |
@pedjak What concerns me about
is that it requires the specification to change to support these use-cases. Especially if we expect the spec and implementations to be stable, we shouldn't require a change to them whenever a new unexpected CR comes into existence months or years from now. |
Sure, I agree - what I meant is that we can come up with some sets of fields we know how to handle and put in the spec. |
I pulled together a proof of concept for my proposal above to use JSONPath to discover containers and JSON Pointer to address pod and container level fields. The implementation works by converting any runtime object into an unstructured form. A mapping definition specifies the location of key elements of the resource necessary for the binding. Using the mapping definition, the unstructured resource is converted into a normalized structured form. That normalized form can then be manipulated to apply the service bindings. Finally, the inverse conversion is applied using the same mapping definition to map the mutated state on to the original object. For the moment, the only aspect of the service binding behavior I have implemented is the detection and defaulting of the There are tests that apply the binding, mapping an object to the meta-model and mapping from the meta-model back to an object are tested on both a PodSpecable resource (Deployment) and non-PodSpecable resource (CronJob). Knowledge of these types is limited to the test suite. There are zero special dependencies. This behavior is implemented using Binding a Deployment with a default mapping: Binding a CronJob with a custom mapping: tl;dr it works |
Application Resource Mapping specifies JSONPath expressions to indentify
containers
,envs
,volumeMounts
, andvolumes
.The unstructured API of apimachinery package expects fields to set values for an object. For example, to set volumes:
I couldn't figure out a way to deterministically resolve the fields from a JSONPath expression. Is that possible? If not, we should reconsider JSONPath expression to specify fields.
The text was updated successfully, but these errors were encountered: