Allow default broker controller to reconcile unset brokerclass.

[liu-cong]

Proposed Changes

• Allow the default broker controller to reconcile brokers without brokerclass annotation for backwards compatibility.

Depends on knative/pkg#1163.

Release Note

NONE

Docs

cc @vaikas

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liu-cong
To complete the pull request process, please assign harwayne
You can assign the PR to them by writing /assign @harwayne in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/broker/controller.go	87.5%	88.9%	1.4

[knative-prow-robot]

@liu-cong: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-build-tests	60a7e50	link	/test pull-knative-eventing-build-tests
pull-knative-eventing-unit-tests	60a7e50	link	/test pull-knative-eventing-unit-tests
pull-knative-eventing-integration-tests	60a7e50	link	/test pull-knative-eventing-integration-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[n3wscott]

It is too easy to get the function wrong. Best to not allow an unset broker class. Use the webhook to set a default.

[liu-cong]

It is too easy to get the function wrong. Best to not allow an unset broker class. Use the webhook to set a default.

Yes we can use webhook for new brokers. However, for existing brokers without an annotation, the reconciler won't pick it up. I think we should make it backwards compatible to ensure a seamless upgrade.

cc @vaikas

[n3wscott]

For existing brokers, the defaulting webhook from the conversion will handle them.

If we want seamless upgrades, we need an upgrade test first.

[liu-cong]

If we want seamless upgrades, we need an upgrade test first.

Yes agree we should have some upgrade tests. I don't think I will be able to add one this week but I will be interested to work on it.

For existing brokers, the defaulting webhook from the conversion will handle them.

What I noticed on my cluster is that the conversion seems to only work on client side, not server side. Although I can see the annotation being added by kubectl get, what the controller sees is still empty.

[knative-prow-robot]

@liu-cong: PR needs rebase.

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.

[liu-cong]

The proper approach is probably to have an upgrade job that handles upgrades for all resources wherever applicable. However we don't have time to design and implement such an upgrade job before 0.14 freezes.

This approach is a quick fix for the issue.

[vaikas]

Yes, proper conversion job would definitely be the proper fix. I do wonder however if in lieu of that work, we could just provide a script that's only going to update brokers because that seems very straightforward and won't have future package with it?
for each namespace
for each broker
apply a patch to add the annotation to ChannelBasedBroker

[liu-cong]

we could just provide a script that's only going to update brokers

@vaikas Do you expect users to manually run this script?

[vaikas]

Yes, that was my thinking. One time upgrade script.

[grantr]

It looks like this is made obsolete by #2910. @liu-cong should this be closed?

[liu-cong]

#2910