servicebinding / spec

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

Create Mappings Extension #145

Open nebhale opened 3 years ago

nebhale commented 3 years ago

Currently, the ServiceBinding type contains a mappings element that allows the creation of mappings from the contents of a ProvisionedService’s Secret to a new Secret projected into an Application. The inclusion of this element, while reducing the number of resources that a user might need to create, had some significant downsides.

Proposal

The mapping element should be removed from the ServiceBinding resource and its functionality should be moved into a set of discrete resource types devoted to this mapping. For example, instead of defining

apiVersion: service.binding/v1alpha2
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    apiVersion: apps/v1
    kind:       Deployment
    name:       online-banking
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service
  mappings:
  - name:  accountServiceUri
    value: https://{{ urlquery .username }}:{{ urlquery .password }}@{{ .host }}:{{ .port }}/{{ .path }}

a user would define multiple resource types

apiVersion: service.binding/v1alpha2
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    apiVersion: apps/v1
    kind:       Deployment
    name:       online-banking
  service:
    apiVersion: mapping.service.binding/v1alpha2
    kind:       GoV1Template
    name:       mapped-prod-account-service

---
apiVersion: mapping.service.binding/v1alpha2
kind: GoV1Template
metadata:
  name: mapped-prod-account-service
spec:
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service
  mappings:
  - name:  accountServiceUri
    value: https://{{ urlquery .username }}:{{ urlquery .password }}@{{ .host }}:{{ .port }}/{{ .path }}

Benefits

This change would immediately improve the permissions situation removing the need for the ServiceBinding reconciler to have permissions to read Secrets. Instead a user could elect whether to add mapping behavior or not based on whether they felt the value/risk of having a third-party controller with access to Secrets was worth it.

If the user wanted mappings, but didn’t trust our implementation this change also provides a point of extensibility allowing a user to install another implementation that they trust more or even implement a mapping reconciler themselves.

That extensibility isn’t just beneficial to users concerned with security either. This same point of extensibility also allows the introduction of an arbitrary number of mapping syntaxes. While we might have implementations for OLM, Go V1 Templates, and more, the project wouldn’t be gatekeepers for platform-specific or user-specific templating syntaxes. Each user could implement or install a mapping controller that suited their needs.

A design that moves mappings out so that they are just another implementation of the ProvisionedService duck type also results in architectural benefits. It improves the separation of concerns (ServiceBinding only cares about performing the binding, not also doing an inline mapping), it simplifies the mapping of the most critical controller, and it enables composability of mappings.

apiVersion: service.binding/v1alpha2
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    apiVersion: apps/v1
    kind:       Deployment
    name:       online-banking
  service:
    apiVersion: mapping.service.binding/v1alpha2
    kind:       TypeProvider
    name:       typed-prod-account-service

---
apiVersion: mapping.service.binding/v1alpha2
kind: TypeProvider
metadata:
  name: typed-prod-account-service
spec:
  service:
    apiVersion: mapping.service.binding/v1alpha2
    kind:       GoV1Template
    name:       mapped-prod-account-service
  type: my-account-service-type
  provider: my-company

---
apiVersion: mapping.service.binding/v1alpha2
kind: GoV1Template
metadata:
  name: mapped-prod-account-service
spec:
  service:
    apiVersion: mapping.service.binding/v1alpha2
    kind:       JSONPath
    name:       extracted-prod-account-service
  mappings:
  - name:  accountServiceUri
    template: https://{{ urlquery .username }}:{{ urlquery .password }}@{{ .host }}:{{ .port }}/{{ .path }}

---
apiVersion: mapping.service.binding/v1alpha2
kind: JSONPath
metadata:
  name: extracted-prod-account-service
spec:
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service
  mappings:
  - name:  username
    path: .status.credentials.username
  - name:  password
    path: .status.credentials.password
  - name:  host
    path: .status.endpoint.host
  - name:  port
    path: .status.endpoint.port
  - name:  path
    path: .status.endpoint.path

Finally by moving this feature, for which each platform has diverging needs, out of the reference implementation it reduces the need to have multiple implementations of the specification which reduces the possibility of collisions within a given cluster.

Outcomes

There are a couple of options for what happens to the mapping functionality after it is extracted.

First, it could become a purely community concern. Each platform vendor would be free to implement their own controllers for their own mapping styles. This option provides ultimate flexibility at the expense of interoperability.

Second, a set of mapping types could be included in the project implementation, but not as part of the specification. This would not preclude vendors from adding their own mapping styles in their platforms. This option maintains much of the flexibility of the previous option while adding some level of interoperability for commonly used mapping needs.

Third, a set of mappings could be included in the specification as well as in the implementation of the specification. This would not preclude vendors from adding their own mapping styles in their platforms. This option creates the highest level of interoperability at the expense of flexibility.

arthurdm commented 3 years ago

My opinion is that mappings as currently defined in the spec does not add very much value, given that the scope of input is limited to the currently exposed Secret - so it's just combining and transforming data that is already available to the application. So +1 in doing something about this. 😄

A new resource similar to the JSONPath sample that @nebhale proposed above would be interesting as it allows the scope to be anything in the source service: .spec, .status, .data, etc, so you can truly add extra value beyond the currently exposed Secret - or in some cases where a Secret is not exposed this feature would create a new one.

In some ways this is the reverse of the binding secret generation strategies, which may be good for cases where the source service's community has no interest in updating their CR.

I think it would be helpful to have, as an extension, at least one of such synthetic mappers (CRD for JSONPath makes sense), so that there's some level of interoperability between opted-in implementations. For example: if we have a JSONPathCR for Strimzi we could re-use that between environments - as long as the implementation supports this extension.

scothis commented 3 years ago

I think it would be helpful to have, as an extension, at least one of such synthetic mappers (CRD for JSONPath makes sense), so that there's some level of interoperability between opted-in implementations.

Do you see value in having different implementations of spec'd mappers? If there are semantic differences in the mapper that would justify distinct implementations, then the value (portability) of them being spec'd is lessened.

arthurdm commented 3 years ago

I think the implementations would be the same - or at least yield the exact same result.

nebhale commented 3 years ago

@arthurdm What's your preference of three possible outcomes? For reference, in the meeting I like #2, but almost everyone else voted for #1 😆

arthurdm commented 3 years ago

I think I would like option 2.5 😄

Have a single CRD for mapping in the extension part of the spec - so it's not required, but if supported by a vendor it will be consistent.

An example (morphed from @nebhale 's examples) of a CR:

apiVersion: mapping.service.binding/v1alpha2
kind: CustomProvisionedService
metadata:
  name: extracted-prod-account-service
spec:
  type:  jsonpath
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service
  mappings:
  - name:  username
    value: .status.credentials.username

So the type of supported template is push into .spec.type so that a single CRD can cover multiple scenarios, and .spec.mappings consistently has {name, value} pairs.

nebhale commented 3 years ago

I believe the "single resource type, multiple type: values" leads us down to needing multiple implementations of the project again. If your platform wants another mapping style, you have to provide a new reconciler for mapping.service.binding/v1alpha2:CustomProvisionedService that will collide with the SIG-provided implementation. Alternatively, having different resources for each mapping style allows disjoint contribution.

arthurdm commented 3 years ago

good point @nebhale - I haven't thought about the reconciler implications.

My preference is for a spec extension to define a set of popular CRDs (JSONPath and Go templates may be good ones to start with, with their unique kind).

johnpoth commented 3 years ago

Hi @nebhale, thanks for including me in this discussion!

151 proposes to be able to define static values in the Provisioned Service's CRD by adding:

“service.binding/key”: "value"

This seems be a requirement for almost all Provisioned Service adopters that will specify a type and provider. Kafka is one example. There are current workarounds already provided by the specification but #151 proposes a simple solution to a simple problem. I think this is important when considering specification adopters.

In this proposal, as you mentioned, this would be addressed in the following way:

apiVersion: service.binding/v1alpha2
kind: ServiceBinding
metadata:
  name: account-service
spec:
  application:
    apiVersion: apps/v1
    kind:       Deployment
    name:       online-banking
  service:
    apiVersion: mapping.service.binding/v1alpha2
    kind:       TypeProvider
    name:       typed-prod-account-service

---
apiVersion: mapping.service.binding/v1alpha2
kind: TypeProvider
metadata:
  name: typed-prod-account-service
spec:
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service
  type: my-account-service-type
  provider: my-company

---

In this solution, the knowledge required to bind a Provisioned Service to an Application is greater:

Whereas with #151, the problem would be addressed by creating a regular ServiceBinding:

apiVersion: service.binding/v1alpha2
kind: ServiceBinding
metadata:
  name: simple-binding
spec:
  application:
    apiVersion: apps/v1
    kind:       Deployment
    name:       online-banking
  service:
    apiVersion: com.example/v1alpha1
    kind:       AccountService
    name:       prod-account-service

Therefore, when considering this use case, as a developer, I would rather use the approach described in #151 than the one described here.

To circle back to the original issue of moving mappings: I agree with the problem but I might not agree with the solution. The mapping problem is a vaste issue and can quickly become a rabbit hole especially if one chooses to solve it in yaml. The current state of the specification proposes a "lightweight" solution to the mapping problem which leverages Go templates.

Beyond "Go templates" use cases, I think the mapping problem should be the responsibility of the application itself. For example, in Java, frameworks, such as Quarkus or Spring, will already read binding properties for you and create Database connections or Kakfa connections with the specification as it is today. This is where the "mapping" between binding properties to service connections is done.

You also added concerns about interoperability (which I share). That and the added complexity this adds to create a ServiceBinding are important concerns when considering adoption from a Provisioned Service/ Application developer perspective. Hence the benefit of such approach isn't so clear compared to the application/framework itself doing the more advanced mapping.

At the very least, I think #151 and #145 should be treated separately?

Let me know what you think,

Thanks !

ps: sorry for the long post...

nebhale commented 2 years ago

The first half of this move (the removal from the core spec) has been completed. The issue is open to track the creation of one or more extension specifications that contain the removed functionality.

baijum commented 2 years ago

I proposed a mapping extension: #221