Remove mappings from the ServiceBinding resource

scothis commented 3 years ago

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

scothis commented 3 years ago

Added a clarification to resolve #155

scothis commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

I just read the docs:

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

baijum commented 3 years ago

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 commented 3 years ago

@baijum thanks for the research

baijum commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

Remove mappings, type and provider from the ServiceBinding resource

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

servicebinding / spec

Remove mappings from the ServiceBinding resource #154