[RFC0030] Add Support for File based Service Binding Information

[beyhan]

This is a tracking issue for the implementation of https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0030-add-support-for-file-based-service-binding.md

Tasks

Beta Give feedback

1. RFC-0030 - Add support to Diego for file based service bindings diego-release#942
   +0
2. RFC-0030 Add support to Diego for file based service bindings bbs#100
   +0
3. RFC-0030 - Add support to Diego for file based service bindings rep#56
   +0
4. RFC-0030 - Add support to Diego for file based service bindings executor#100
   +0
5. [RFC0030 - 1] Add column file_based_service_bindings_enabled to apps table cloud_controller_ng#3996
   +0
6. [RFC0030 - 2] File-based service bindings app feature cloud_controller_ng#3997
   +0
7. [RFC0030 - 3] File-based service bindings cloud_controller_ng#4026
   +0
8. Technical Specification: Translation of VCAP_SERVICES to Service Binding Files cloud_controller_ng#4036
   +0

[dimitardimitrov13]

Hello there, here are the 4 PRs within diego-release, bbs, rep, and executor. They are needed for RFC-0030.

1. Diego Release: RFC-0030 - Add support to Diego for file based service bindings diego-release#942
2. BBS: RFC-0030 Add support to Diego for file based service bindings  bbs#100
3. REP: RFC-0030 - Add support to Diego for file based service bindings rep#56
4. Executor: RFC-0030 - Add support to Diego for file based service bindings  executor#100

[beyhan]

@dimitardimitrov13 thanks for the update! I updated the tasks list.

[dimitardimitrov13]

@beyhan, thank you.

[ameowlia]

🎉 Hi @dimitardimitrov13 , Excellent work so far!

Please ping me on slack (too easy to miss github notifications with 100+ repos 🫠 ) when the CAPI PRs are done and we can test this end-to-end. At that point I will assign a Diego reviewer.

[dimitardimitrov13]

Hello Amelia,

Thanks for your message. Unfortunately, we do not have a straightforward way to test this end-to-end. During the implementation stage, I used a custom build of cfdot and a mocked code inside the BBS to simulate this. The code was cleaned up from the BBS before the PR. It seems we will need to wait for the CAPI team.

Best regards,
Dimitar

[PlamenDoychev]

Hi @ameowlia,

Just in case you haven't seen the input from @dimitardimitrov13 above.
Should we wait for CAPI then?

[beyhan]

Should we wait for CAPI then?

This is what I read in the comment above #901 (comment)

[ameowlia]

Yes, I want to wait to review and merge until we can test it end-to-end. That way we can make sure everything works all the way through and we don't have to worry about breaking changes if part of it is released, but then it ends up things need to change to work with CAPI.

[philippthun]

@beyhan, @dimitardimitrov13: As I've started to implement the CAPI part of this, I came across the changed BBS interface. I suggest that we adapt Files to File as this entity represents a single file. This would be inline with e.g. EnvironmentVariable.

When already changing the BBS interface, I would also like to discuss, if we can adapt the attributes. Currently they are named name and value. I would find path and content more intuitive. path due to the fact that we always specify files within a parent folder named after the binding, so the value would be something like my-binding/instance_name. Maybe relative_path would be even better...

[philippthun]

And another topic to discuss is the usage of non-Ascii characters. Today you can do the following:

cf create-user-provided-service 💥 -p '{"💣":"🔥"}'

The env var looks then as follows:

VCAP_SERVICES: {
  "user-provided": [
    {
      "binding_guid": "1389756a-016a-4dac-80c6-e0a71b80c8e3",
      "binding_name": null,
      "credentials": {
        "💣": "🔥"
      },
      "instance_guid": "ed10c46a-3d75-4832-b53b-294ac7cfae09",
      "instance_name": "💥",
      "label": "user-provided",
      "name": "💥",
      "syslog_drain_url": null,
      "tags": [],
      "volume_mounts": []
    }
  ]
}

With file-based service bindings this would result in a file $SERVICE_BINDING_ROOT/💥/💣. Besides contradicting the Service Binding for Kubernetes spec (Binding names MUST match [a-z0-9-.]{1,253} / The name of a binding entry file name SHOULD match [a-z0-9-.]{1,253}), I'm wondering if this would work. What are the specs for directory and file names in the tmpfs file system?

[philippthun]

There's also a possible name conflict. The RFC states that the directory should be named after the service binding. I guess that this refers to the name attribute, which is either binding_name or (if the first one is not set) instance_name. Whereas it is not possible to create two service instances with the same name or two service bindings with the same name, it is possible to create a first service binding with the name foobar from an arbitrary service instance and another service binding without a name from a service instance called foobar. This would mean that both binding entries end up with name=foobar. When mapping this to file-based service bindings, there would be a conflict...

[beyhan]

@beyhan, @dimitardimitrov13: As I've started to implement the CAPI part of this, I came across the changed BBS interface. I suggest that we adapt Files to File as this entity represents a single file. This would be inline with e.g. EnvironmentVariable.

When already changing the BBS interface, I would also like to discuss, if we can adapt the attributes. Currently they are named name and value. I would find path and content more intuitive. path due to the fact that we always specify files within a parent folder named after the binding, so the value would be something like my-binding/instance_name. Maybe relative_path would be even better...

Referring to a discussion (#994 (comment)) in an RFC willing to build on top of this I would say that we should go for the suggested name there change ServiceBindingFiles to VolumeMountedFiles and Files to File. The suggestion to change name to path and value to content sounds reasonable to me. If everyone is OK with this I will update the RFC. @ameowlia , @ChrisMcGowan , @rkoster , @stephanme, @Gerg, @PlamenDoychev, @dimitardimitrov13 any thoughts on this?

[dimitardimitrov13]

Hello,

I agree that good naming conventions should always be chosen. Since we have already changed it once (as requested by Stefan Merker), it could be done again. Regarding the other comments, I'll take a look next week and get back to you with answers.

Best regards,
Dimitar

[beyhan]

And another topic to discuss is the usage of non-Ascii characters. Today you can do the following:

cf create-user-provided-service 💥 -p '{"💣":"🔥"}'

The env var looks then as follows:

VCAP_SERVICES: {
  "user-provided": [
    {
      "binding_guid": "1389756a-016a-4dac-80c6-e0a71b80c8e3",
      "binding_name": null,
      "credentials": {
        "💣": "🔥"
      },
      "instance_guid": "ed10c46a-3d75-4832-b53b-294ac7cfae09",
      "instance_name": "💥",
      "label": "user-provided",
      "name": "💥",
      "syslog_drain_url": null,
      "tags": [],
      "volume_mounts": []
    }
  ]
}

With file-based service bindings this would result in a file $SERVICE_BINDING_ROOT/💥/💣. Besides contradicting the Service Binding for Kubernetes spec (Binding names MUST match [a-z0-9-.]{1,253} / The name of a binding entry file name SHOULD match [a-z0-9-.]{1,253}), I'm wondering if this would work. What are the specs for directory and file names in the tmpfs file system?

The RFC defines that we should support the K8s service binding spec naming conventions in

community/toc/rfc/rfc-0030-add-support-for-file-based-service-binding.md

Line 36 in 411475b

2. Implement the K8s service binding specification. The environment variable `SERVICE_BINDING_ROOT` defines the location for the service bindings. The name of the file and the format follow the [K8s specification](https://servicebinding.io/):

. I would suggest that the CC validates this and returns an error if it is not the case. This will be a breaking change for users' with naming which is not supported by the K8s spec but the users must anyway opt-in here and in this case it should be fine. I couldn't find any specific rules for tmpfs file system but most probably it supports the same as Linux OS and according to the rules listed in https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations K8s naming specification should be fine. Any thoughts on this @ameowlia , @ChrisMcGowan , @rkoster , @stephanme, @Gerg?

[beyhan]

There's also a possible name conflict. The RFC states that the directory should be named after the service binding. I guess that this refers to the name attribute, which is either binding_name or (if the first one is not set) instance_name. Whereas it is not possible to create two service instances with the same name or two service bindings with the same name, it is possible to create a first service binding with the name foobar from an arbitrary service instance and another service binding without a name from a service instance called foobar. This would mean that both binding entries end up with name=foobar. When mapping this to file-based service bindings, there would be a conflict...

In the K8s spec is stated that:

Users SHOULD ensure each binding has a unique name. The behavior for name collisions is undefined. Implementations MAY attempt a good faith check for collisions to provide a meaningful error message.

I think we should make sure that this conflict results in an error like with the name in the previous comment. It also sounds as a missing validation in the current behaviour of CC. @philippthun Do we have any other alternative?

[philippthun]

And another topic to discuss is the usage of non-Ascii characters. [...]

I would suggest that the CC validates this and returns an error if it is not the case. [...]

Okay, I've implemented the MUST requirement for service binding names. But what about the SHOULD requirement for binding entry file names?

We would already violate this by using the defined VCAP_SERVICES attributes as file names, e.g. binding_guid, as the underscore is not allowed by the k8s spec. Should we:

1. translate underscores to dashes (e.g. binding_guid to binding-guid) and then strictly enforce the k8s spec?
2. add the underscore to the list of allowed characters and enforce this slightly modified spec?
3. simply ignore this SHOULD requirement...

I would prefer one of the first two options.

[philippthun]

There's also a possible name conflict. [...]

I think we should make sure that this conflict results in an error [...]

Agree - I've implemented this check.

[beyhan]

And another topic to discuss is the usage of non-Ascii characters. [...]

I would suggest that the CC validates this and returns an error if it is not the case. [...]

Okay, I've implemented the MUST requirement for service binding names. But what about the SHOULD requirement for binding entry file names?

We would already violate this by using the defined VCAP_SERVICES attributes as file names, e.g. binding_guid, as the underscore is not allowed by the k8s spec. Should we:

1. translate underscores to dashes (e.g. binding_guid to binding-guid) and then strictly enforce the k8s spec?
2. add the underscore to the list of allowed characters and enforce this slightly modified spec?
3. simply ignore this SHOULD requirement...

I would prefer one of the first two options.

Hi @philippthun,

I would say that we should go with option 1. because we aim to reuse existing tooling from the CNCF community here and it could be that they fail if we aren't conform with the K8s spe.

@ameowlia , @ChrisMcGowan , @rkoster , @stephanme, @Gerg any opinion on this?

[beyhan]

I opened #1008 to address all this discussions.