Service binding resolver #228

menehune23 · 2021-09-15T21:04:18Z

Summary

Adds a servicebindings.Resolver type which can be used to lazily load and resolve service bindings according to the k8s spec.

Implementation notes:

Because the new spec relies on a $SERVICE_BINDING_ROOT env var, this PR maintains backwards compatibility by falling back to $CNB_BINDINGS, then <platform>/bindings if neither env var is set.
Bindings can be read from either the new k8s format or the legacy format.
Rather than providing top-level functions, this PR uses a dedicated type so that consumers can create interfaces for it and also mock/fake it in unit tests to keep them simple (for instance, this eliminates the need to understand the spec's implementation and write files to the filesystem during unit tests that rely on a binding).
Entry values are of a new io.ReadCloser-compatible type, Entry -- rather than simply a string or []byte -- that can stream the entry from the file system. It also supports .ReadString() and .ReadBytes() for ease of use. (Credit to @ryanmoran for the great idea).

Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
I have added an integration test, if necessary.
I have reviewed the styleguide for guidance on my code quality.
I'm happy with the commit history on this PR (I have rebased/squashed as needed).

Closes #107

bindings/resolver.go

postal/internal/dependency_mappings_test.go

postal/service.go

bindings/resolver.go

postal/internal/dependency_mappings_test.go

bindings/resolver.go

postal/internal/fakes/binding_resolver.go

postal/fakes/mapping_resolver.go

bindings/resolver.go

servicebindings/entry.go

postal/internal/dependency_mappings.go

postal/service.go

postal/service_test.go

menehune23 · 2021-09-24T00:44:14Z

servicebindings/resolver.go

+//   2. CNB_BINDINGS environment variable, if above is not set
+//   3. `<platformDir>/bindings`, if both above are not set
+func (r *Resolver) Resolve(typ string, provider string, platformDir string) ([]Binding, error) {
+	if newRoot := bindingRoot(platformDir); r.bindingRoot != newRoot {


Now that the platform is part of this method: If the binding root changes, we'll refresh and cache new bindings. Note that this implicitly occurs on a new resolver, due to the root being initialized to "".

servicebindings/resolver.go

ryanmoran · 2021-09-24T16:00:54Z

I'm happy with where this is heading now. @sophiewigmore do you have anything else you'd like to see before we think about getting this merged?

menehune23 · 2021-09-24T16:33:30Z

I'm happy with where this is heading now. @sophiewigmore do you have anything else you'd like to see before we think about getting this merged?

@ryanmoran @sophiewigmore Just let me know before you merge, and I'll clean up the commit history

sophiewigmore · 2021-09-24T21:52:09Z

@ryanmoran @menehune23 hey y'all. sorry I missed the update earlier. I've been following along with these discussions and this sounds good to me!

ForestEckhardt · 2021-09-27T16:07:03Z

@menehune23 I am still a little unsure about the removal of the postal.Install function. I know that it is deprecated but the removal of the function entirely feels like it would necessitate a major bump as we are changing a part of the API by removing functionality. If our goal is to not do a major bump then I feel like that would bee to be added back in unless I am missing something or the intention is to do a major bump and I have just missed the boat on the decision.

menehune23 · 2021-09-27T16:30:11Z

@menehune23 I am still a little unsure about the removal of the postal.Install function. I know that it is deprecated but the removal of the function entirely feels like it would necessitate a major bump as we are changing a part of the API by removing functionality. If our goal is to not do a major bump then I feel like that would bee to be added back in unless I am missing something or the intention is to do a major bump and I have just missed the boat on the decision.

@ForestEckhardt That's a good point. I'll restore it and its tests until later.

Signed-off-by: Brayan Henao <bhenao@vmware.com>

menehune23 changed the base branch from bindings to main September 15, 2021 21:04

menehune23 force-pushed the bindings branch from ff66737 to 30fc702 Compare September 15, 2021 21:05

menehune23 mentioned this pull request Sep 15, 2021

Adds basic service bindings support #226

Closed

5 tasks

menehune23 force-pushed the bindings branch 4 times, most recently from f996636 to c05d818 Compare September 15, 2021 21:38

menehune23 commented Sep 15, 2021

View reviewed changes

bindings/resolver.go Outdated Show resolved Hide resolved

menehune23 force-pushed the bindings branch from c05d818 to e8ee253 Compare September 15, 2021 21:43

menehune23 commented Sep 15, 2021

View reviewed changes

postal/internal/dependency_mappings_test.go Outdated Show resolved Hide resolved

menehune23 requested a review from ryanmoran September 15, 2021 23:52

menehune23 commented Sep 15, 2021

View reviewed changes

postal/service.go Outdated Show resolved Hide resolved

menehune23 force-pushed the bindings branch from 4ae6333 to 76b3032 Compare September 17, 2021 00:19

menehune23 marked this pull request as ready for review September 17, 2021 18:06

menehune23 requested a review from a team as a code owner September 17, 2021 18:06

menehune23 requested a review from paketo-bot-reviewer September 17, 2021 18:06