Remove mappings from the ServiceBinding resource

[scothis]

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.

The .spec.type and .spec.provider fields are also removed, as
they are syntactic sugar on top of mappings.

Refs #145
Resolves #155

Signed-off-by: Scott Andrews andrewssc@vmware.com

[baijum]

LGTM

[scothis]

Added a clarification to resolve #155

[baijum]

a mapping to enrich

@scothis
Are you referring to the "mappings" as described in #145?
Alternatively, the .spec.type (optional) value available in the ServiceBinding resource can be used in the binding Secret, right? Should we add that info into the note here?

[scothis]

.spec.type is removed by this PR

[baijum]

I just noticed that now :-)

The removal of .spec.type and .spec.provider should be called out in the PR description because I don't see mentioning it in #145.

BTW, I am fine with removing .spec.type and .spec.provider. That way, the reconciler can mount the Secret resource into the application workload, and no need to create an intermediate Secret resource.

I hope soon the extensions mentioned in #145 will become a reality!

[scothis]

The removal of .spec.type and .spec.provider should be called out in the PR description

updated the PR subject and overview

I hope soon the extensions mentioned in #145 will become a reality!

nothing to wait for. Start creating and using them today; they're just not part of the spec, yet(?)

[scothis]

Based on the call today, we want to reevaluate removing the .spec.type and .spec.provider fields. The thought is that we can create additional volumes/volumemounts for these specific files when needed. Before taking this approach, we need to ensure Kubernetes volume mounts can be safely nested, and what happens when there is a collision (e.g. the type exists, but is being overridden).

[baijum]

I tried the following configuration but seems to be not working. This is the error message in the pod:

OCI runtime create failed: container_linux.go:367: starting container process caused:
process_linux.go:495: container init caused: rootfs_linux.go:60:
mounting "/var/lib/kubelet/pods/4f575521-2efb-4a06-9ea0-1ba93dfe1927/volume-subpaths/type/busybox/1"
to rootfs at 
"/var/lib/docker/overlay2/30f1880fedd229c6bda44de1eda54038751dc49f2456ff6a15821732f6b9eab5/merged/bindings/type" 
caused: not a directory: unknown: Are you trying to mount a directory onto a file (or vice-versa)?
Check if the specified host path exists and is the expected type

---
apiVersion: v1
kind: Secret
metadata:
  name: secret1
type: Opaque
stringData:
  provider: "backingservice"
  username: "guest"
  password: "passwd"
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm1
data:
  type: custom
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: app
  labels:
    environment: test
spec:
  selector:
    matchLabels:
      environment: test
  template:
    metadata:
      labels:
        environment: test
    spec:
      containers:
        - name: "busybox"
          image: "busybox:latest"
          command: ["sleep"]
          args: ["1h"]
          volumeMounts:
            - name: bindings
              mountPath: /bindings
            - name: type
              mountPath: /bindings/type
              subPath: type
      volumes:
        - name: bindings
          secret:
            secretName: secret1
        - name: type
          configMap:
            name: cm1
            items:
              - key: type
                path: type

[baijum]

I just read the docs:

Volumes can not mount onto other volumes or have hard links to other volumes.

[baijum]

I tried to use life cycle hooks after setting readOnly: false in the volumeMounts. But due to security reasons setting readOnly to false has no impact, it will be always readOnly: true.

...
          volumeMounts:
            - name: bindings
              mountPath: /bindings
              readOnly: false
            - name: type
              mountPath: /extra-bindings/type
              subPath: type
          lifecycle:
            postStart:
              exec:
                command:
                  - /bin/sh
                  - -c
                  - ln -sf /extra-bindings/type /bindings/type
...

This is the error message:

PostStartHookError: command '/bin/sh -c ln -sf /extra-bindings/type /bindings/type'
exited with 1: ln: /bindings/type: Read-only file system

[scothis]

@baijum thanks for the research

[baijum]

@scothis A projected volume maps several existing volume sources into the same directory.

Here is a modified example:

---
apiVersion: v1
kind: Secret
metadata:
  name: secret1
type: Opaque
stringData:
  type: "to-be-changed"
  provider: "backingservice"
  username: "guest"
  password: "passwd"
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cm1
data:
  type: custom
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: app
  labels:
    environment: test
spec:
  selector:
    matchLabels:
      environment: test
  template:
    metadata:
      labels:
        environment: test
    spec:
      containers:
        - name: "busybox"
          image: "busybox:latest"
          command: ["sleep"]
          args: ["1h"]
          volumeMounts:
            - name: all-in-one
              mountPath: /bindings
      volumes:
        - name: all-in-one
          projected:
            sources:
              - secret:
                  name: secret1
              - configMap:
                  name: cm1
                  items:
                    - key: type
                      path: type

As you can see the original value for type in the Secret resource is getting overridden by the value in the ConfigMap.
I confirmed it:

$ kubectl exec -it app-774f8fcd48-mp9g9 -- cat /bindings/type 
custom

I tried to change some values in the Secret resource, and that is getting reflected in the file system immediately.

So, we are good to go with merging this PR :-)

Edit: typo

[baijum]

we are good to go with merging this PR

Based on the above information, we can retain .spec.type and .spec.provider as optional fields. I think we can add an additional note about these special fields in the spec. Something like this:

Note: Users can employ the `.spec.type` and `.spec.provider` fields to override the original
values in the Provisioned Service Secret resource. Mainly, it would be helpful if the
Provisioned Service Secret resource doesn't provide those values.

(Feel free to rephrase)

[baijum]

The Application Projection section 2nd paragraph also needs some re-write. This is how it currently reads:

The Secret data MUST contain a type entry with a value that identifies the abstract classification of the binding. The Secret type (.type verses .data.type) MUST reflect this value as service.binding/{type}, replacing {type} with the Secret data type. It is RECOMMENDED that the Secret data also contain a provider entry with a value that identifies the provider of the binding. The Secret data MAY contain any other entry.

[scothis]

@baijum updated to restore the type and provider fields on the ServiceBindingSpec. Removed references to "secrets" in the application projection as a Secret is more of an implementation detail at this point.

[baijum]

LGTM

[baijum]

I implemented the ProjectedVolumeSource:
https://github.com/kubepreset/kubepreset/blob/51b7446417e3ac88499291af4d488650f67c5a59/controllers/binding/servicebinding_controller.go#L185

And tested it:
https://github.com/kubepreset/kubepreset/blob/51b7446417e3ac88499291af4d488650f67c5a59/controllers/binding/servicebinding_controller_test.go#L513

[baijum]

Remove mappings, type and provider from the ServiceBinding resource

@scothis Please update the title of this PR: type and provider is not removed.

[arthurdm]

I noticed some key words (should / must) weren't in the proper format, and I think the wording was a bit confusing in terms of what the spec is saying must be present (which is the type in the projection, not the secret)

Suggested change

	> Note: While the Provisioned Service referenced `Secret` data should contain a `type` entry, the `type` must be defined before it is projected into an application workload. This allows a mapping to enrich an existing secret.
	> Note: While the Provisioned Service referenced `Secret` data **SHOULD** contain a `type` entry, the `type` **MUST** be present in the application projection.

[scothis]

this is a non-normative block, so it intentionally uses unstylized "should" and "must". The spec requirements are captured in the Application Projection section.

The driver for this note is #155