Disable the autogenerated 'default' mt broker

[matzew]

Let's please DISABLE the "auto MT broker injection" for the 0.14 release.

Potential issues

1. both (channel and mt) "default brokers" are called default:
   I understand the name should have no impact. That's good, but let's than not auto generate all MT brokers, if we are NOT explicitly enable it. See 2)

2. Currently the "channel broker" is default, however, regardless the MT-broker still generates a lot of brokers (for all namespaces), with the name default. This prevents the kubectl label namespace default knative-eventing-injection=enabled to create a "channel broker" for my namespace. because the MT broker was already there 😞

3. Because it still generates all these brokers, but they are not functioning, see:

k get brokers --all-namespaces
NAMESPACE          NAME      READY   REASON                 URL                                                        AGE
default            default   False   EndpointsUnavailable   http://default-broker.default.svc.cluster.local            8s
knative-eventing   default   False   EndpointsUnavailable   http://default-broker.knative-eventing.svc.cluster.local   7s
knative-serving    default   False   EndpointsUnavailable   http://default-broker.knative-serving.svc.cluster.local    7s
kourier-system     default   False   EndpointsUnavailable   http://default-broker.kourier-system.svc.cluster.local     7s
kube-node-lease    default   False   EndpointsUnavailable   http://default-broker.kube-node-lease.svc.cluster.local    8s
kube-public        default   False   EndpointsUnavailable   http://default-broker.kube-public.svc.cluster.local        8s
kube-system        default   False   EndpointsUnavailable   http://default-broker.kube-system.svc.cluster.local        8s

And, like stated in 2. I can NOT generate a "non mt" broker, with the normal "injection" knative-eventing-injection=enabled, because the MT broker did already create one instance of itself for me, in all my namespaces.

** With this PR **

MT broker is "completely disabled", but can be deployed to knative. Has no undesired sideeffect.

Turning it on, is like:

1. change the config-br-defaults ConfigMap and set the value of the brokerClass to MTChannelBasedBroker
2. change the mt-broker-controller Deployment, and set the BROKER_INJECTION_DEFAULT value to true.

Now, all "default" MT brokers are good:

k get brokers --all-namespaces
NAMESPACE          NAME      READY   REASON   URL                                                                                 AGE
default            default   True             http://broker-ingress.knative-eventing.svc.cluster.local/default/default            10s
knative-eventing   default   True             http://broker-ingress.knative-eventing.svc.cluster.local/knative-eventing/default   12s
knative-serving    default   True             http://broker-ingress.knative-eventing.svc.cluster.local/knative-serving/default    10s
kourier-system     default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kourier-system/default     12s
kube-node-lease    default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-node-lease/default    10s
kube-public        default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-public/default        11s
kube-system        default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-system/default        11s

When both brokers are deployed, with the following config:

channelBased configured default
mt-channel-broker is NOT the default, but has BROKER_INJECTION_DEFAULT set to true

There are a couple of issues:

• both brokers are called default.

I get that we want that, but this kinda really prevents, that the mt-broker should

due to conflicts with regular 'default' broker

NAMESPACE          NAME      READY   REASON                 URL                                                        AGE
default            default   True                           http://default-broker.default.svc.cluster.local            2m5s
knative-eventing   default   False   EndpointsUnavailable   http://default-broker.knative-eventing.svc.cluster.local   22s
knative-serving    default   False   EndpointsUnavailable   http://default-broker.knative-serving.svc.cluster.local    23s
kourier-system     default   False   EndpointsUnavailable   http://default-broker.kourier-system.svc.cluster.local     23s
kube-node-lease    default   False   EndpointsUnavailable   http://default-broker.kube-node-lease.svc.cluster.local    22s
kube-public        default   False   EndpointsUnavailable   http://default-broker.kube-public.svc.cluster.local        22s
kube-system        default   False   EndpointsUnavailable   http://default-broker.kube-system.svc.cluster.local        23s

Signed-off-by: Matthias Wessendorf mwessend@redhat.com

[matzew]

I think another reason for initially disabling this, is that this new feature is handy, but not fully battle tested ?

[matzew]

Thoughts ?

@mattmoor @vaikas @grantr

[antoineco]

Why not always creating a broker (channel or mt, doesn't matter) when and only when that label is present?

[matzew]

no change on the label - that just generally prevents that namespace to have an injected broker

[matzew]

This now also works with:

1. using CH-Broker, and do the labeling (two pods for "default" ch-broker, in my namespace (default ns))
2. changing CFGs to go w/ MT broker.

Now we have MT brokers and one CH-broker, see:

k get brokers --all-namespaces
NAMESPACE          NAME      READY   REASON   URL                                                                                 AGE
default            default   True             http://default-broker.default.svc.cluster.local                                     101s
knative-eventing   default   True             http://broker-ingress.knative-eventing.svc.cluster.local/knative-eventing/default   12s
knative-serving    default   True             http://broker-ingress.knative-eventing.svc.cluster.local/knative-serving/default    13s
kourier-system     default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kourier-system/default     12s
kube-node-lease    default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-node-lease/default    13s
kube-public        default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-public/default        13s
kube-system        default   True             http://broker-ingress.knative-eventing.svc.cluster.local/kube-system/default        12s

[antoineco]

Seems sensible. Thanks Mat!

I'm really not a fan of this auto-injection feature based on BROKER_INJECTION_DEFAULT. From the bottom of my head I can't think of a scenario where someone would want to enable a broker in every namespace, regardless of its cost. Most clusters I've seen in production have just as many system namespaces as user namespaces. System namespaces are not labeled out of the box. User namespaces are unlikely to be all relying on Kn Eventing.

Also I've seen a lot of people (ab)using kubectl get all. Questions will be asked when BROKER_INJECTION_DEFAULT is set to true.

But that's a different discussion.

/lgtm

[matzew]

@antoineco some context / background: knative/docs#2325 (comment)

[matzew]

/hold

[aslom]

I agree with @antoineco and @matzew about default broker injection - knative/docs#2325 (comment)

And it is easy to enable default in next release but harder to disable it (avoiding surprising changes).

/lgtm

[mattmoor]

I'm not going to "fight the current" here this close to a release, but I want to talk principles because it feels like the MT Broker DevX is suffering because folks want it to co-exist with the ST Broker, which has "licked" the proverbial "cookie".

On the use of Namespace reconcilers...

Namespace reconcilers (in my experience) exist to pre-provision things that optimize the developer experience. K8s default ServiceAccounts's are a great example of something that they could force folks to create manually. In Serving we have an optional namespace controller to pre-provision wildcard certificates per namespace to optimize the expenditure of Let's Encrypt quota and eliminate TLS provisioning time.

My belief was that this feature in Eventing aspired to be a convenience that pre-created Brokers so that folks could create Triggers on it without another thought. This comment from @grantr echos that:

the original purpose of the namespace magic was to work around the per-broker requirement of additional deployments, services, RBAC, etc. It seems like this doesn't apply to the MT broker since it doesn't create any additional per-Broker objects

Honestly, because of the need to opt-in per occurrence, the use of:

kubectl label namespace default knative-eventing-injection=enabled

... vs. simply kubectl create -nNAMESPACE -f broker.yaml has always felt a bit broken, but aspirational to me. The aspiration being exactly what mtnamespace does today.

On the path forward...

Like I said, I'm not going to "fight the current" this close to a release, but I'd at least like to understand the path forward in 0.15.

If we can agree that this is a desirable DevX, then it sounds like it has to be opt-in, but I think that mechanism for opting-in should be like nscert in Serving, where you can install a separate controller and opt-out per-Namespace. This would require us to shift our documentation for creating Brokers to actually... create the Brokers 😲

I'm uncomfortable with the current impasse where ST Broker has "licked the cookie" on the namespace DevX, can't actually support the goal state in its current form, and where I haven't seen a tenable plan to make that happen.

[mattmoor]

Honestly, if creating a broker becomes:

kubectl apply -nNAMESPACE -f - <<EOF
apiVersion: eventing.knative.dev/v1beta1
kind: Broker
metadata:
  name: default
spec: {}
EOF

I think it'd be an improvement over explicitly labeling the namespace. I have to look up the label every. damn. time. 🤷‍♂️

[matzew]

@mattmoor the MT broker should be IMO the default in 0.15, see: #2935

I just listed out here, some issues with both installed etc. so. keeping the ST broker default for now, and use 0.15 timeframe to work towards MT broker, as the default.

[mattmoor]

@matzew shut up and take my money.

I think that's great, but somewhat orthogonal to this. If we want ST Broker to remain viable even as an alternative, then I think the issues you raise are important, which is why I wanted to level set on our goals with namespace-based Brokers.

[matzew]

If we want ST Broker to remain viable even as an alternative

I think at least for a little bit ?

I'd be also fine in marking it as deprecated in 0.15, and kill it with fire for 0.16 ?

So, perhaps?
0.14 lands MT broker (disabled)
0.15 enables it (as default). Says deprecated to ST broker
0.16 ST says adios

[matzew]

@mattmoor does that sound like a reasonable agenda ? ^

[grantr]

Thanks everyone for your thoughts on this PR! Seems like the question of whether we want to opt in or opt out is orthogonal. I'm all for continuing the discussion in #2937, but this issue is more specific: we have two things that are fighting over the default cookie and that leads to an unintended confusing user experience. We can decide which thing gets the cookie separately, but for 0.14 @matzew's proposed approach seems like a good stopgap.

[grantr]

/lgtm

(Leaving the hold to give everyone a chance to weigh in)

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, matzew

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [grantr,matzew]

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

[mattmoor]

0.14 lands MT broker (disabled)
0.15 enables it (as default). Says deprecated to ST broker
0.16 ST says adios

This SGTM. I think there are also options that unblock the semantics we want without forcing ST Broker into early retirement. Happy to help game those out.

[matzew]

ok, mind to “unhold” this PR ?

On Thu 9. Apr 2020 at 19:18, Matt Moore ***@***.***> wrote: 0.14 lands MT broker (disabled) 0.15 enables it (as default). Says deprecated to ST broker 0.16 ST says adios This SGTM. I think there are also options that unblock the semantics we want without forcing ST Broker into early retirement. Happy to help game those out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2946 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTR72HMLSXTNTBA7EWLRLX7OFANCNFSM4MERY7PQ> .

-- Sent from Gmail Mobile

[mattmoor]

/unhold