search

servicebinding / spec

Specification for binding services to k8s workloads
https://servicebinding.io
Apache License 2.0
92 stars 35 forks source link

Revamp ClusterWorkloadResourceMapping #183

Closed scothis closed 2 years ago

scothis commented 3 years ago

The mapping structure now more closely aligns with the PodTemplateSpec shape. It uses a JSONPath query for container-like structures that may occur anywhere within the target workload resource, and JSON Pointer references to specific structures either from the root of the object or relative to a container discovered by a path query.

The high level approach is described in https://github.com/k8s-service-bindings/spec/issues/177#issuecomment-884935203 and with proof of concept code in https://github.com/k8s-service-bindings/spec/issues/177#issuecomment-887538101

Resolves #177 #173 #189

scothis commented 3 years ago

This will need to be updated when #148 is merged

scothis commented 3 years ago

I kept the raw JSON Pointer syntax for now. I wanted to get the semantics of this change nailed before we talk about syntax sugar.

baijum commented 3 years ago

I kept the raw JSON Pointer syntax for now.

As we discussed in the last meeting, let's use a restricted JSONPath expression. I think we can use a definition something like this:

The value should be a sequence of Dot selectors as defined in the JSONPath draft document. https://datatracker.ietf.org/doc/html/draft-ietf-jsonpath-base-01 (Refer section 3.5.2)

Based on this definition, the PodSpec-able volume can be expressed like this:

.spec.template.spec.volumes
scothis commented 2 years ago

Any objection to merging this? There's a draft implementation in servicebinding/service-binding-controller#41 that shows the proposed changes working.

arthurdm commented 2 years ago

thanks @scothis. The only concern I have is the different syntax between the path definition of the containers (using . as the separator) versus the path definition of the volumes (using / as the separator). I think they should be consistent.

scothis commented 2 years ago

@arthurdm see https://github.com/servicebinding/spec/pull/183#issuecomment-902058333 and https://github.com/servicebinding/spec/pull/183#pullrequestreview-758029393. We also discussed this on the community call and agreed to track it in #189 as a follow up to this PR. I agree that we don't want to tag a release as is.

arthurdm commented 2 years ago

I am definitely fine to defer additional / built-on capabilities to subsequent PRs, as that allows us to move faster in steps, but I get concerned when we introduce something in a PR that we don't fully agree with and think needs to change. There is always the risk that the subsequent change never makes it in.

scothis commented 2 years ago

Added a commit to this PR to define a replacement for JSONPointer.

The draft implementation has also been updated to use this new syntax.

arthurdm commented 2 years ago

Added a commit to this PR to define a replacement for JSONPointer. The draft implementation has also been updated to use this new syntax.

Thanks @scothis!