Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add watching on Secrets in drcluster_controller #1167

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Add watching on Secrets in drcluster_controller #1167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion controllers/drcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,13 @@ func (r *DRClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {

return ctrl.NewControllerManagedBy(mgr).
For(&ramen.DRCluster{}).
Watches(&source.Kind{Type: &corev1.ConfigMap{}}, handler.EnqueueRequestsFromMapFunc(r.drClusterConfigMapMapFunc)).
Watches(&source.Kind{Type: &ramen.DRPlacementControl{}}, drpcMapFun, builder.WithPredicates(drpcPred())).
Watches(&source.Kind{Type: &ocmworkv1.ManifestWork{}}, mwMapFun, builder.WithPredicates(mwPred)).
Watches(&source.Kind{Type: &viewv1beta1.ManagedClusterView{}}, mcvMapFun, builder.WithPredicates(mcvPred)).
Watches(&source.Kind{Type: &corev1.ConfigMap{}}, handler.EnqueueRequestsFromMapFunc(r.drClusterConfigMapMapFunc)).
Watches(&source.Kind{Type: &corev1.Secret{}}, handler.EnqueueRequestsFromMapFunc(r.drClusterSecretMapFunc),
builder.WithPredicates(util.CreateOrDeleteOrResourceVersionUpdatePredicate{}),
).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is little bit inconsistent with other code - in other watches we have a function (e.g. mcvMapFun) and predicate (e.g. mcvPred). Here we have harder to follow structure.

Copy link
Member Author

@ELENAGER ELENAGER Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, here is the same structure
I moved the other watch with the same structure, hope it looks more clear now

Complete(r)
}

Expand All @@ -141,6 +144,24 @@ func (r *DRClusterReconciler) drClusterConfigMapMapFunc(configMap client.Object)
return requests
}

func (r *DRClusterReconciler) drClusterSecretMapFunc(secret client.Object) []reconcile.Request {
if secret.GetNamespace() != NamespaceName() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not clear what is this namespace. Not an issue in your change but maybe it is time to make this more clear.

return []reconcile.Request{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return nil? (empty slices is nil)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am returning it for the consistency with the other code. The same return can be found in 5 more places in this file.

}

drcusters := &ramen.DRClusterList{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to create the DRClusterList on the heap - we can do:

drclusters := ramen.DRClusterList{}
r.Client.List(context.TODO(), &drcusters)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are probably right, but it's also for consistency. We are always creating lists on heap (I've looked through all our code).

if err := r.Client.List(context.TODO(), drcusters); err != nil {
return []reconcile.Request{}
}

requests := make([]reconcile.Request, len(drcusters.Items))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of reconciling all DRClusters we may want to filter out only those that reference the secret via their S3Profile name. The same non-optimized version exists here as well, so let's add a TODO/issue at least to keep this tracked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an optimization issue

for i := range drcusters.Items {
requests[i].Name = drcusters.Items[i].GetName()
}

return requests
}

// drpcPred watches for updates to the DRPC resource and checks if it requires an appropriate DRCluster reconcile
func drpcPred() predicate.Funcs {
drpcPredicate := predicate.Funcs{
Expand Down Expand Up @@ -263,6 +284,7 @@ func filterDRClusterMCV(mcv *viewv1beta1.ManagedClusterView) []ctrl.Request {
// +kubebuilder:rbac:groups=apps.open-cluster-management.io,resources=placementrules,verbs=get;list;watch
// +kubebuilder:rbac:groups=cluster.open-cluster-management.io,resources=placements,verbs=get;list;watch
// +kubebuilder:rbac:groups=argoproj.io,resources=applicationsets,verbs=get;list;watch
// +kubebuilder:rbac:groups="",resources=secrets,verbs=list;watch
// +kubebuilder:rbac:groups="",resources=configmaps,verbs=list;watch

func (r *DRClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
Expand Down