Skip to content
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

Create Mappings Extension #145

Open
nebhale opened this issue Mar 3, 2021 · 10 comments
Open

Create Mappings Extension #145

nebhale opened this issue Mar 3, 2021 · 10 comments

Comments

@nebhale
Copy link
Member

nebhale commented Mar 3, 2021

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.

  • The ServiceBinding reconciler needed to have permissions to read existing Secrets.
  • A single fixed set of mapping syntaxes had to be defined
  • If a platform wanted additional or alternate mapping syntaxes they had to create their own ServiceBinding reconciler and ensure that only a single implementation was installed.

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
Copy link
Member

arthurdm commented Mar 4, 2021

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
Copy link
Contributor

scothis commented Mar 5, 2021

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
Copy link
Member

arthurdm commented Mar 5, 2021

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

@nebhale
Copy link
Member Author

nebhale commented Mar 17, 2021

@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
Copy link
Member

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
Copy link
Member Author

nebhale commented Mar 17, 2021

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
Copy link
Member

arthurdm commented Mar 17, 2021

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
Copy link

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:

  • Knowledge of TypeProvider and variants
  • Knowledge that the type and provider fields not exposed by the Provisioned Service
  • Knowledge that application needs the type and provider fields

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...

scothis added a commit to scothis/service-binding-specification that referenced this issue May 14, 2021
The existing mappings on the ServiceBinding resource were introduced as
a way to make it easier for a user to enrich an existing Secret into a
form appropriate for binding to an application workload. This approch
had a few issues that can be better addressed by other resources that
interoperate with the ServiceBiding resource. The issues include:
- the ServiceBinding controller needs to be able to read and write
  Secrets
- Go templates were used to compose new values, which worked for basic
  templating, but were limited in their capabilities
- the capabilities of the Go templates are an implemenation detail of
  how the controller is built and could change over time independent of
  the speced behavior.

The behavior applied by mappings can be reintroduced as dedicated
resources that can themselves expose a Secret as a ProvisionedService,
which can be consumed by a ServiceBinding. This change further separates
the concerns of provisioning a service from binding a service.

Refs servicebinding#145

Signed-off-by: Scott Andrews <[email protected]>
@nebhale nebhale added this to the mappings/1.0.0-alpha1 milestone Sep 16, 2021
@nebhale nebhale changed the title Moving Mappings Create Mappings Extension Mar 3, 2022
@nebhale
Copy link
Member Author

nebhale commented Mar 3, 2022

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
Copy link
Contributor

baijum commented Jul 28, 2022

I proposed a mapping extension: #221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants