Drop application label selector

[scothis]

ServiceBinding must specify the application to bind by name.

This is a discussion-draft alternative to #179, which is trying to create symmetry between app and service references by adding a label selector to the service reference. This PR creates that same symmetry by removing support for label selectors in application references.

[baijum]

This PR creates that same symmetry by removing support for label selectors in application references.

I think "symmetry" is not a good reason to remove the application label selector. The service label selector was creating an M-N relation and there are other unresolved issues discussed in #179. The application label selector doesn't have the same issues. .

[arthurdm]

I strongly feel that we need to be consistent with our support for workloads and services - not just for the sake of symmetry, but because there are mirror use cases for using labels in both sides.

My recommendation is that we merge this PR and properly design how to support label selectors for both workloads and services via #184

[scothis]

...not just for the sake of symmetry, but because there are mirror use cases for using labels in both sides.

There is a meaningful difference. Service label selector behavior can be solved by resources that decompose to ServiceBindings or with a *Mapping resource. There's no alternate behavior for the workload selector, removing it from the spec is a net loss in functionality.

While the ServiceBinding resource's structure is symmetrical for services and workload, the runtime behavior is not symmetrical. The reconciler needs to read from the referenced service, while it needs to write into the referenced workload.

[arthurdm]

Service label selector behavior can be solved by resources that decompose to ServiceBindings or with a *Mapping resource

Both of these solutions are not in the spec today. One could say the same thing for application label selectors: "you can solve this with resources the decompose to ServiceBindings".

[scothis]

One could say the same thing for application label selectors

Mostly, but not completely. For example binding to an immutable resource (like a Job) with a generated name. The binding can only happen during admission for the create request, and because it's not named, it can only be matched with a label selector. I fully admit it's a corner case, but the behavior cannot be replicated by decomposing to ServiceBinding resources.

[arthurdm]

I am not sure immutable resources are even covered by our spec, because even without labels in the pictures, what if I have a named immutable resource created, and then afterwards I create a ServiceBinding CR that tries to bind a service to it?

[scothis]

We have a sample that uses a Job. It works so long as the ServiceBinding is defined first. https://github.com/vmware-labs/service-bindings/blob/main/samples/overridden-type-provider/README.md

[arthurdm]

It works so long as the ServiceBinding is defined first

Right - which is not a restriction the spec makes. So we cannot claim that we need workload label functionality in ServiceBinding to support immutable resources, because the spec does not make any claims or guarantees on the ordering of workload / service / ServiceBinding CR, which means we may not always be able to catch the create mutation hook.

[arthurdm]

In the last community hangout we agreed on 3 steps:

• close PR #179
• merge this PR (#182)
• work on an overall solution for labels that applies to both workloads and services via issue #184

Can we please re-open and proceed with this PR?

[scothis]

Apologies, it was premature to close this PR.

I don't think we agreed to merge this PR on the last call. We agreed to continue the discussion on the issue. Based on the comments so far, both @baijum and I are leaning 👎.

There are alternative ways to handle a service selector without addressing it in the core spec, while the same is not true for the workload selector. If we remove it from the spec there is a loss of functionality. If we agree that simplifying the spec is worth the loss of functionality we can move forward with this PR.

[arthurdm]

while the same is not true for the workload selector.

That's the main assumption I want to keep challenging. The only argument so far was about immutable workload CRs, but we have settled that it's a problem related to the order of k8s CR creation (e.g. needing ServiceBinding to come before the workload we're binding), not related to labels. You would have the same order-restriction problem with a named (not labelled) immutable resource.

I think we need to make it a "first principle" that a ServiceBinding CR is always 1-to-1 between a workload and a service, and higher level CRs (via #184) would decompose into individual ServiceBinding CRs. If immutable resources are in the picture, that's another orthogonal story about ordering, for either named or labelled resources.

[scothis]

I think we need to make it a "first principle" that a ServiceBinding CR is always 1-to-1 between a workload and a service, and higher level CRs (via #184) would decompose into individual ServiceBinding CRs.

The point I'm trying to make, which we seem to be continually missing each other on is that is we embrace this principle, then there is a loss of functionality on the workload side that cannot be compensated for with a higher level resource. This loss of behavior is not true on the service side because it can be compensated for with a higher level resource.

We're at a point in the lifecycle of the spec where I think we need to bring PoC code to a design discussion that is controversial (for example https://github.com/k8s-service-bindings/spec/issues/177#issuecomment-887538101). Since you think it's possible to compensate for removing the workload selector with a higher level resource, please provide an example of such a resource and the rough semantics.

[arthurdm]

That's fair - I've now added a sample under https://github.com/k8s-service-bindings/spec/issues/184#issuecomment-904879075

there is a loss of functionality on the workload side that cannot be compensated for with a higher level resource.

I think that can be easily done via https://github.com/k8s-service-bindings/spec/issues/184#issuecomment-904879075 - this is what I mean by a higher level resource, and why I think it covers both camps (workload and service). I wonder if you had the reverse in mind: a ServiceBinding CR referring to another resource.

[baijum]

a higher-level resource

I am fine with a higher-level resource as an extension to generate ServiceBinding resources based on label selectors. But that necessarily does need not remove the label selector from the ServiceBinding resource.

The higher-level resource can mandate to generate the ServiceBinding resources with names for workloads and services. Then mandate to treat the generated ServiceBinding resources as immutable (probably based on a special annotation).

I hope this approach will meet the requirements from @arthurdm and not lose the functionality pointed by @scothis.

[arthurdm]

thanks @baijum

My view is that we won't have loss of functionality with the proposal in #184. The same type of implementation details (outside of the spec mandate) can be made to the proposal in order to support the edge case of injecting into a immutable resource. So thus there would be no need to keep the label selector for workloads in ServiceBinding.

[baijum]

My view is that we won't have loss of functionality with the proposal in #184.

It does lose the functionality from the core spec. And I don't see a reason to remove this feature from the core spec.

[arthurdm]

There won't be loss of functionality if the proposal from #184 becomes part of the core spec.

If we decide that #184 is better as an extension, and we are concerned with loss of functionality, then we really should revisit #179 and work on enhancing it to remove unclarity.

Leaving the spec only catering for half of the label use cases would not be a good outcome.

[baijum]

There won't be a loss of functionality if the proposal from #184 becomes part of the core spec.

My bad. I thought the proposal was to create an extension with a higher-level resource to generate the ServiceBinding resources.

Hypothetically speaking, when that higher-level resource is part of the core spec, I still see value for label selectors in the ServiceBinding resource. Because in most of the cases ServiceBinding resource with a label selector would be sufficient. And the added complexity of yet another resource doesn't warrant the removal of a feature from the ServiceBinding resource. So, let's continue to discuss the higher-level resource without removing the label selector from the ServiceBinding resource.

[arthurdm]

My main concern with not dropping the label selector is that we're catering for the workload use cases but not the service use cases (from https://github.com/k8s-service-bindings/spec/issues/169). I don't think the spec should be bias towards one versus the other just because of implementation details.

So I see it as two choices: (a) Reword #179 to be clear and implementable for both workloads and services in the ServiceBinding CRD. I truly believe this is implementable if we word it carefully. (b) Remove workloads label from ServiceBinding CRD and just have them live in the ServiceBindingSelector CRD

[scothis]

My main concern with not dropping the label selector is that we're catering for the workload use cases but not the service use cases (from #169). I don't think the spec should be bias towards one versus the other just because of implementation details.

The spec isn't biased toward one use case over the other. It's implementing as much as is practical and well defined. The problem space is not symmetrical.

Using a label selector to match multiple workloads is well defined. Using a label selector to match multiple services is not. As an implementor, I know how to write a workload label selector that works robustly (and have done so). I do not know how to write a service label selector that isn't full of edge cases and foot guns. I've asked for you to define the semantics for these edge cases several times, and you haven't. I don't enjoy being a stick in the mud, but there are practical issues with the proposal for service bindings that haven't been addressed.

Back to the top of this PR:

I don't think merging this PR is a good idea because it involve a loss of functionality that is not otherwise readily achievable. That said, I don't mind simplifying the spec to remove a feature where the runtime/implementation costs outweigh the benefits; but that's not what's being argued here.

So I see it as two choices:

There's a third choice: leave the core spec as is, prototype new behavior in an extension.

[arthurdm]

I am definitely surprised this PR has gotten so lengthy. 😄

I thought the issue with the service label PR (#179 ) as we discussed in the hangout wasn't a question about workload vs services, it was about the proposal to "pick the first match", which I agree has edge cases. But if we wanted to keep ServiceBinding 1-to-1 between workload and service then it's either picking the first match or use something like the #184 proposal.

If we're saying we're OK with multiple mappings in ServiceBinding, which is where the spec is right now with workload labels, but we don't know how to implement service labels, I think that's a different problem.

Besides the mounted path needing a suffix similar to #184 , what is the challenge in implementing service label in ServiceBinding with multiple matches (on par with workloads)? Let's work through those.

[arthurdm]

@scothis - as per our chat today, I am ok with this PR being closed.