SinkBinding doesn't work with KServices that uses BYO revision names

[rhuss]

Describe the bug

When using a SinkBinding with a Knative Service as subject that uses BYO revision names (i.e. setting the revision name from the client), then no K_SINK environment variable is injected to the KService.

Expected behavior

K_SINK should be injected to the Pod Template as a container env var regardless whether a Knative Service is configured for BYO revision names or not.

To Reproduce

kn service create random --image rhuss/random:1.0
kn broker create default
kn source binding create my-sb --subject Service:serving.knative.dev/v1:random --sink broker:default
kn service describe random --verbose | grep K_SINK

This will get no result as kn used BYO revision names by default. If switching over to server side generated names, then a K_SINK env var is injected.

# Switch over to server side generated names
kn service update random --revision-name ""
kn service describe random --verbose | grep K_SINK

        Env:    K_SINK=http://broker-ingress.knative-eventing.svc.cluster.local/default/default

Knative release version

Eventing 0.17.3 (running on minikube)

[rhuss]

See also discussion at https://knative.slack.com/archives/C9JP909F0/p1600876741003800

[evankanderson]

This is probably a serving, rather than eventing, issue -- it would apply equally to a hypothetical OsbBinding which set the VCAP_SERVICES environment variable on a Knative Service which was in the "BYO revision" mode.

I wonder if the best solution would be to clear spec.template.metadata.name on the resource before applying the patch, or adding a default to clear that field into the patch if the patch doesn't explicitly manage that field.

@mattmoor @dprotaso for thoughts.

[mattmoor]

Ultimately, different PodSpecables present different restrictions. For some cases they can handle it gracefully, and for others they should probably fail gracefully.

I'd characterize BYO name as one of our niche restrictions.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[rhuss]

I'd characterize BYO name as one of our niche restrictions.

Until 0.20, BYO name was the default mode for kn (don't remember why this was the case), we changed that now to server-side naming in 0.21. However, since we still support the older kn version and it is still easy to switch to BYO revision names with kn, maybe this should be supported by the binding reconciliation loop? E.g. one could easily detect BYO mode and, if enabled, just set the revision name to a new random name.

As an alternative, I would suggest deprecated BYO revision name and let it fade out. I really don't see any convincing argument for BYO names, especially if it can not be ruled out, that only a single client is used for manipulating a KService.

I'm reopening this issue, just to track this open question.

[cardil]

I hit that one with kn 0.19 and serving 0.21. Maybe do reopen as you said @rhuss ?

/reopen
/remove-lifecycle stale

[knative-prow-robot]

@cardil: Reopened this issue.

In response to this:

I hit that one with kn 0.19 and serving 0.21. Maybe do reopen as you said @rhuss ?

/reopen
/remove-lifecycle stale

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[markusthoemmes]

Is there anything left to be done here? The above sounds like this is working as designed and we'd at most document the behavior better and/or improve visibility of the issue?