Introduce addressable resolver aggregated cluster role

[devguyio]

Fixes #916

Proposed Changes

• Add addressable-resolver aggregated cluster role
• Add broker addressable cluster role
• Add channel addressable cluster role

Release Note

Introduce a new aggreagted ClusterRole for Addressable.

[devguyio]

/assign @Harwayne

[Harwayne]

Looks good!

I think we should add read permissons for K8s and Knative Services as well, as those tend to be the other things that need to be resolved.

[n3wscott]

Awesome!!

[n3wscott]

I think eventing.knative.dev/addressable -> duck.knative.dev/addressable

@Harwayne what do you think?

[devguyio]

@n3wscott I like duck.knative.dev/addressable more indeed. More generic, so not just eventing so can be used for non eventing addressable, and it conveys the addressable nature of being a duck typing interface.

[Harwayne]

I missed Knative routes, which should also be included.

And we can remove part of 200-controller-clusterrole.yaml and add a binding for this new ClusterRole to eventing-controller in 201-clusterrolebinding.yaml.

[devguyio]

@Harwayne

I missed Knative routes, which should also be included.

Done

And we can remove part of 200-controller-clusterrole.yaml and add a binding for this new ClusterRole to eventing-controller in 201-clusterrolebinding.yaml.

After some thought I chose not to change the controller clusterrole defined in 200-controller-clusterrole.yaml since the addressable resolver ClusterRole is readonly , and basically this change would result in removing 3 lines for (kservices and routes), adding a whole new rolebinding AND have some overlapping rules between two role bindings for no real benefit :/ .

so I think for sake of simplicity and have all needed context in a single role, I think maybe it's better to keep it as it is.

[Harwayne]

After some thought I chose not to change the controller clusterrole defined in 200-controller-clusterrole.yaml since the addressable resolver ClusterRole is readonly , and basically this change would result in removing 3 lines for (kservices and routes), adding a whole new rolebinding AND have some overlapping rules between two role bindings for no real benefit :/ .

so I think for sake of simplicity and have all needed context in a single role, I think maybe it's better to keep it as it is.

My preference is the opposite. You're completely correct that it doesn't buy us much now, but I think it will be a great benefit in the medium term. I want all pieces that need to resolve Addressables to use the single aggregated role for that purpose. Then they can have their own unique roles for everything else they need. My reasoning is that the big benefit of the aggregated role is so that when new Addressable CRDs are created, then the existing infrastructure that needs to resolve them immediately gets the permission to do so, without needing to do any work.

For example, I create a new CRD named Foo that is Addressable and add a ClusterRole to read Foos that is aggregated into the aggregate ClusterRole. I then create a Subscription whose spec.subscriber.ref is a Foo.

In the current, single ClusterRoleBinding world, the Subscription controller can't process that Subscription, because it doesn't have permission to read Foos.

If instead, the Subscription controller had the aggregated ClusterRole, then it would work immediately, without needing to grant any new bindings.

[devguyio]

@Harwayne done

[Harwayne]

/lgtm

[Harwayne]

/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Abd4llA, Harwayne

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 [Harwayne]

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