buildpacks / libcnb

A non-opinionated language binding for the Cloud Native Buildpack Buildpack and Extension specifications
Apache License 2.0
32 stars 13 forks source link

Creating bindings from VCAP_SERVICES fails if credentials are nested #254

Closed modulo11 closed 9 months ago

modulo11 commented 10 months ago

The credentials field contained in VCAP_SERVICES is specified as:

A JSON object containing the service-specific credentials needed to access the service instance

When a Binding is created from VCAP_SERVICES credentials are expected as map[string]string (flat) where in reality they can be nested (map[string]any). A Binding expects them to be map[string]string as well. Even worse, translating VCAP_SERVICES into Bindings swallows any marshaling error.

Simple VCAP_SERVICES to reproduce the issue:

{
  "elephantsql-provider": [
    {
      "name": "elephantsql-binding-c6c60",
      "label": "elephantsql-type",
      "credentials": {
        "uri": "postgres://postgres.example.com",
        "foo": {
          "bar": 42
        }
      }
    }
  ]
}

There are a couple potential solutions that came to my mind:

stephprat commented 10 months ago

For my use case, ignoring nested is fine.

dmikusa commented 10 months ago

Even worse, translating VCAP_SERVICES into Bindings swallows any marshaling error.

Definitely a bug.

When a Binding is created from VCAP_SERVICES credentials are expected as map[string]string (flat) where in reality they can be nested (map[string]any). A Binding expects them to be map[string]string as well.

My understanding is that the K8s binding spec says that they should be mapped that way. Key/value. String as the key and string as the value. That said, I did some reading in the spec and didn't find it specifically saying that. I spent about five mins looking though.

What I do know is that the binding spec maps the bindings to files on the file system, so the key definitely needs to be a string, because it'll be a file name. In theory, the file could contain anything, but it can't be nested. You can't have subfolders within the binding.

Flatten incoming credentials, which would just cut of any nested structures. From the example, only the uri field would be inside the secret field of Binding. This would also prevent the whole process of translating to either fully succeed or just silently fail.

I'm fine with this. To clarify, you're saying that it would have properties like this:

Ex: username -> foo Ex: foo.bar -> 42

I am a little concerned with how we'd implement this. We'd probably have to use map[string]any then convert it to a map[string]string as we go through and flatten things. I think it could work, but it's probably going to be very tedious.

Change type of thesecret property in Binding from map[string]string to map[string]any. That would be a breaking change with lots of impact on other projects I guess.

-1 I don't think this is an option. Too much change, too many things impacted that are never going to have this problem.

Treat credentials as raw data which is passed and "interpreted" by consuming parties. Would probably lead to even more work downstream

Not 100% sure I follow this one. I'd be OK with this if you're saying that it remains string/string and that the value, if containing nested data, would be just a JSON string.

Ex: username -> foo is still string/string. Ex: foo -> {"bar": 42} still string/string. If the consuming entity needs bar then it's their responsibility to parse that JSON and read out bar.

What I don't know, and what might be an issue, is if Go's JSON parsing would do that. I suspect it would fail on the nested item since it's a string.

Other options?

  1. Just ignore properties that are not strings
  2. Fail the whole thing if there are any properties that are not string

I don't know what a use case would be where it has nested credentials though. Do you have a use case where there are nested values under the credentials? Can you expand on the need here?