[RFC] Add Support for File based Service Binding Information

[beyhan]

This RFC suggests to introduce an alternative option for the CF platform to provide Service Binding information to applications.

Preview

[mogul]

Some app developers are going to want to make the minimum changes possible, and 1 definitely works there. Other app developers are going to porting an app from k8s, and 2 makes their path onto CF easier. Is it possible to offer both 1 and 2, and have developers pick which environment variable they'll use to find the file?

[dmikusa]

My read is that users will be able to pick between using the existing env variable approach, which means zero change for their apps, or they can use the file-based approach which would mean some level of change for their app with either approach. So there is control for the user that doesn't want change.

Both of the implementations above sound like there would be work involved for an app to adopt. Minimally, you're going to need to know to read from a file instead. It's small, but if you've got apps that are not actively being developed, even that may be more work than you're willing to do.

Not sure if it needs to go into this proposal, but I would suggest that we target the common libraries that folks are using to read VCAP_SERVICES, like java-cfenv (I believe there are Python & Node.js libraries also) and get support for both formats, env variable and file, and then it's a lot less relevant how it's implemented. The libraries will smooth things over and possibly even make it so that no code change is required at the app level.

[beyhan]

My read is that users will be able to pick between using the existing env variable approach, which means zero change for their apps, or they can use the file-based approach which would mean some level of change for their app with either approach. So there is control for the user that doesn't want change.

This is correct. @mogul given this clarification do you still see benefits in supporting both options in parallel?

Not sure if it needs to go into this proposal, but I would suggest that we target the common libraries that folks are using to read VCAP_SERVICES, like java-cfenv (I believe there are Python & Node.js libraries also) and get support for both formats, env variable and file, and then it's a lot less relevant how it's implemented. The libraries will smooth things over and possibly even make it so that no code change is required at the app level.

@dmikusa Do you mean here option 1) or both? Most probably both but I wanted to check?

[dmikusa]

Both. I think regardless of what we do, we can lean on the libraries to help with smoothing things over for hopefully a lot of users.

[UlfFildebrandt]

Just to comment on the reason for both options: There is a problem with the length of VCAP and running with existing VCAP + K8s service binding would not simplify to use the libraries to get the variables. So, to run both alternatives in parallel, I would propose to store the variables in both formats in two different files, not only keep the current VCAP in memory. This would solve the existing problem and also prepare for future service bindings.

[PlamenDoychev]

2️⃣

[stephanme]

2️⃣

[ameowlia]

2️⃣

[ChrisMcGowan]

2️⃣

[beyhan]

Thank you for the votes! I didn't set an end date for the voting but most of the RFC reviewers and the TOC already voted and the winner is clear. That is why, I updated the RFC (88f5fb6) with the winning option. Please review the latest state.

[rkoster]

It would be good to add a section describing how the name and tags OSBAPI fields could be mapped to type and provider service binding fields (if these are not specified by the broker), addressing initial broker compatibility. This could for example be done based on well-known tags like postgres and mysql. This would be in line with the service-specific code that is currently used to populate the DATABASE_URL variable.

[dmikusa]

In libcnb for Cloud-Native Buildpacks, we implemented a translation from VCAP_SERVICES to CNB bindings. What we did was this:

1. Credentials map was turned into key/value files (i.e. key is the file name, value is the file contents). We did not go into nested maps, so if a key in Credentials had a nested JSON value, then the JSON is written as the contents of the file.
2. We used the Label from VCAP_SERVICES as the type.
3. We used the top-level key from the VCAP_SERVICES map as the provider.

I haven't used it myself, but I know people are using it. @loewenstein - How has this mapping worked out in practice?

[loewenstein]

We contributed that to libcnb mainly because of the Paketo Dynatrace Buildpack and for that it works - as far as I can tell.

[beyhan]

@loewenstein is buildpacks/libcnb#228 you are referring to? The mentioned mapping above isn't directly visible in the change but most probably I have to dive deeper into it.

[dmikusa]

I think there were two or three PRs to arrive at the present implementation. You can see what we have settled on here.

[beyhan]

Looking into the implementation of the CC the top-level key from the VCAP_SERVICES seems to be also the label from the VCAP_SERVICES. The value is taken from the name attribute of the service offering as defined in the OSBAPI spec. I think it make sense to be consistent and have the same mapping as implemented in libcnb and will update the RFC with this if we agree to go with the 2) approach and don't here anything against the suggested mapping.

[beyhan]

I added details about the VCAP_SERVICES -> K8s bindings mapping with 88f5fb6. Please review and provide feedback.

[stephanme]

Adding app features to the cf app manifest has been requested before for enabling ssh: cloudfoundry/cloud_controller_ng#2996

The proposal should allow to explicitly enable and disable a feature flag (i.e. not just by listing the flag). Otherwise it would be impossible to disable a feature flag because the CF app manifest is not declarative but additive (see https://v3-apidocs.cloudfoundry.org/#apply-a-manifest-to-a-space).

E.g.

applications:
- name: testapp
  features:
  - file-based-service-bindings: true

or, closer to the app-features v3 API,

applications:
- name: testapp
  features:
  - name: file-based-service-bindings
    enabled: true

[beyhan]

I agree and tried to address this with 0ae1408. Is that OK?

[stephanme]

From concept point of view, it looks ok. But there is one yaml error in the 2nd proposal (see my comment below).

[stephanme]

"name" is missing before file-based-service-bindings in the alternative proposal:

features:
  - name: file-based-service-bindings
    enabled: true

[beyhan]

Thanks! Fixed with de5b48f

[stephanme]

From concept point of view, it looks ok. But there is one yaml error in the 2nd proposal (see my comment below).

[stephanme]

2️⃣

[beyhan]

We are starting the FCP with the goal to approve this RFC by next Tuesday 7th of May.