operator-sdk icon indicating copy to clipboard operation
operator-sdk copied to clipboard

Add support for ClusterRole AggregationRule during bundle generation

Open zimnx opened this issue 5 months ago • 12 comments

Description of the change:

ClusterRoles that use an AggregationRule often do not have any rules defined directly. Instead, their rules are aggregated from other ClusterRoles that match the AggregationRule’s label selector.

The existing generator logic only included rules from ClusterRoles that were explicitly bound via ClusterRoleBindings to the ServiceAccounts used by Deployments. Since ClusterRoles with an AggregationRule typically lack direct rule definitions, the resulting permission bundle ended up being empty.

Motivation for the change:

Improve user experience of bundle generator for users using ClusterRole AggregationRule.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Fixes #6977

zimnx avatar Aug 05 '25 16:08 zimnx

@zimnx - please sign the DCO.

mykaul avatar Aug 10 '25 13:08 mykaul

@zimnx - please sign the DCO.

Done

@jberkhahn @rashmigottipati Kind reminder about this PR, I would appreciate feedback.

zimnx avatar Aug 11 '25 07:08 zimnx

This is on our path for OpenShift certification. It'd be great if it can get reviewed.

mykaul avatar Aug 14 '25 11:08 mykaul

@grokspawn perhaps you could take a look? It didn't get much attention. Thanks in advance!

zimnx avatar Aug 20 '25 15:08 zimnx

Along with the unit test changes, we'd also want to update our integration tests to make sure that an operator with these rules in their config/rbac has a bundle that is generated properly, and that the operator can run in a cluster. to update the unit integration tests, you can look at hack/generate/samples/internal/go/memcached-with-customization/memcached_with_customization.go to see how we post process the operator's directories and add in the necessary things to fully test different scaffoling options, ontop of what you get from kubebuilder (which operator-sdk is based on).

Thanks for hints, I've added ClusterRole having AggregatedRule to integration tests.

zimnx avatar Aug 28 '25 12:08 zimnx

@zimnx Can you create a new changlog fragment as an addition for this? The initial comment lays out how to accomplish that.

acornett21 avatar Aug 28 '25 16:08 acornett21

Can you create a new changlog fragment as an addition for this? The initial comment lays out how to accomplish that.

yes, added.

zimnx avatar Aug 29 '25 09:08 zimnx

Hi @jberkhahn @rashmigottipati @acornett21 @joelanford

~All comments have been addressed and the tests are passing - are we good to merge this?~

mflendrich avatar Sep 12 '25 09:09 mflendrich

Hi @jberkhahn @rashmigottipati @acornett21 @joelanford

All comments have been addressed and the tests are passing - are we good to merge this?

Hi @mflendrich All comments haven't been addressed, the below is still open https://github.com/operator-framework/operator-sdk/pull/6978#discussion_r2334704456

acornett21 avatar Sep 12 '25 14:09 acornett21

Hi @zimnx If you'd still like this PR to land in this repo, could you address the comment from Sept 9th? Thanks

  • https://github.com/operator-framework/operator-sdk/pull/6978#discussion_r2334704456

acornett21 avatar Nov 04 '25 14:11 acornett21

Hi @acornett21,

Speaking on behalf of the project that has active interest in getting this change upstreamed (that @zimnx has recently left, but the project remains) - I'll be happy to pick this up soon (no later than next week).

Unless you intend to take this yourself soon, @zimnx? In that case I'll stay out of the way.

mflendrich avatar Nov 04 '25 14:11 mflendrich

@acornett21 - fyi, I've picked this up and responded in https://github.com/operator-framework/operator-sdk/pull/6978#discussion_r2528695678.

mflendrich avatar Nov 14 '25 19:11 mflendrich