stolostron / klusterlet-addon-controller

Apache License 2.0
4 stars 22 forks source link

Add label when GRC is deployed #50

Closed itdove closed 3 years ago

itdove commented 3 years ago

https://github.com/open-cluster-management/backlog/issues/14944 Signed-off-by: Dominique Vernier dvernier@redhat.com

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: itdove

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/open-cluster-management/klusterlet-addon-controller/blob/main/OWNERS)~~ [itdove] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
itdove commented 3 years ago

/hold

sonarcloud[bot] commented 3 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

tpouyer commented 3 years ago

@qiujian16 I asked @itdove to create this PR so please don't blame him ;-)

We (cloud-services team) need to use the Placement API in our client-side operator to allow a user to "select" a set of managed clusters for which they wish us to configure identity providers. We need the ability, through the Placement API to identify just those Managed Clusters that have the GRC PolicyController addon enabled. Since I cannot find any examples of a Placement Predicate that is using anything other than cluster labels we needed to identify a way to add a label to indicate if grc's PolicyController addon is enabled on the ManagedCluster. This PR would solve that problem for us by looking at the KlusterletAddonConfig and updating the ManagedCluster with the label policycontroller.addon.open-cluster-management.io equal to true. The ability to automatically add labels for other klusterlet addons that are enabled may be something to investigate but we only need to know about the grc PolicyController for our use case.

If the Placement predicate does support us matching an expression for enabled/disable addons I'm happy to use that instead but I'd need a link to some code that I could read to understand how to use it. If there is no support for such an expression I'd really like you to review this PR and we can discuss if we can get this merged in for the 2.4 release.

Attn: @mdelder @robinbobbitt

qiujian16 commented 3 years ago

@tpouyer I think having placement to select clusters based a certain set of addons is enabled or not is a valid use case. Placement API does not support it today, but I would want this to be an enhancement in placement.

@itdove would you create an issue in https://github.com/open-cluster-management-io/community/issues with this use case?

itdove commented 3 years ago

/unhold

itdove commented 3 years ago

Do you plan to lgtm this PR or we must put the issue I will create as a blocker for our project?

tpouyer commented 3 years ago

@qiujian16 I've opened an issue in the project here: https://github.com/open-cluster-management-io/community/issues/74

@itdove let's close this PR for now and can you work with @qiujian16 to figure out how best to implement solution for the issue https://github.com/open-cluster-management-io/community/issues/74 as quickly as we can.

@qiujian16 until we have a solution for this the work I'm doing with @itdove is blocked and cannot make progress. How quickly can I get a resolution for this issue?

qiujian16 commented 3 years ago

I will take a look at it today. IF it blocks you, I think we can merge this as a workaround for now. I think we'd better add some comments on the code to indicate this is a workaround.

We have to pipeline the placement enhancement in the community, I think there is another enhancement about global placement that is also needed? Let's also create an issue for that...

tpouyer commented 3 years ago

I will take a look at it today. IF it blocks you, I think we can merge this as a workaround for now. I think we'd better add some comments on the code to indicate this is a workaround.

We have to pipeline the placement enhancement in the community, I think there is another enhancement about global placement that is also needed? Let's also create an issue for that...

That sounds like a good plan... merge this PR to give us a work-around and then you can take more time to figure out the right approach for allowing the user of addon enabled/disabled status in the predicate cluster selector.