Skip to content

Commit edf340b

Browse files
karlkfikarlkfi
authored
chore: minor cleanup to reconilcer controllers (#1563)
1 parent 0eedab6 commit edf340b

File tree

5 files changed

+139
-153
lines changed

5 files changed

+139
-153
lines changed

‎pkg/reconcilermanager/controllers/reconciler_base.go‎

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"errors"
2020
"fmt"
2121
"path/filepath"
22+
"strconv"
2223
"strings"
2324
"time"
2425

@@ -31,6 +32,7 @@ import (
3132
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3233
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
3334
"k8s.io/apimachinery/pkg/runtime"
35+
"k8s.io/apimachinery/pkg/runtime/schema"
3436
"k8s.io/apimachinery/pkg/types"
3537
"k8s.io/apimachinery/pkg/util/json"
3638
"k8s.io/client-go/dynamic"
@@ -50,6 +52,7 @@ import (
5052
webhookconfiguration "kpt.dev/configsync/pkg/webhook/configuration"
5153
"sigs.k8s.io/controller-runtime/pkg/client"
5254
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
55+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
5356
)
5457

5558
const (
@@ -103,8 +106,8 @@ type reconcilerBase struct {
103106
githubApp githubAppSpec
104107
webhookEnabled bool
105108

106-
// syncKind is the kind of the sync object: RootSync or RepoSync.
107-
syncKind string
109+
// syncGVK is the GroupVersionKind of the sync object: RootSync or RepoSync.
110+
syncGVK schema.GroupVersionKind
108111

109112
// controllerName is used by tests to de-dupe controllers
110113
controllerName string
@@ -601,7 +604,7 @@ func (r *reconcilerBase) setupOrTeardown(ctx context.Context, syncObj client.Obj
601604
controllerutil.AddFinalizer(syncObj, metadata.ReconcilerManagerFinalizer)
602605
if err := r.client.Update(ctx, syncObj, client.FieldOwner(reconcilermanager.FieldManager)); err != nil {
603606
err = status.APIServerError(err,
604-
fmt.Sprintf("failed to update %s to add finalizer", r.syncKind))
607+
fmt.Sprintf("failed to update %s to add finalizer", r.syncGVK.Kind))
605608
r.logger(ctx).Error(err, "Finalizer injection failed")
606609
return err
607610
}
@@ -650,7 +653,7 @@ func (r *reconcilerBase) setupOrTeardown(ctx context.Context, syncObj client.Obj
650653
controllerutil.RemoveFinalizer(syncObj, metadata.ReconcilerManagerFinalizer)
651654
if err := r.client.Update(ctx, syncObj, client.FieldOwner(reconcilermanager.FieldManager)); err != nil {
652655
err = status.APIServerError(err,
653-
fmt.Sprintf("failed to update %s to remove the reconciler-manager finalizer", r.syncKind))
656+
fmt.Sprintf("failed to update %s to remove the reconciler-manager finalizer", r.syncGVK.Kind))
654657
r.logger(ctx).Error(err, "Removal of reconciler-manager finalizer failed")
655658
return err
656659
}
@@ -686,7 +689,7 @@ func (r *reconcilerBase) updateRBACBinding(ctx context.Context, reconcilerRef, r
686689
} else if rb, ok := binding.(*rbacv1.RoleBinding); ok {
687690
rb.Subjects = subjects
688691
}
689-
core.AddLabels(binding, ManagedObjectLabelMap(r.syncKind, rsRef))
692+
core.AddLabels(binding, ManagedObjectLabelMap(r.syncGVK.Kind, rsRef))
690693
if equality.Semantic.DeepEqual(existingBinding, binding) {
691694
return nil
692695
}
@@ -708,7 +711,7 @@ func (r *reconcilerBase) upsertSharedClusterRoleBinding(ctx context.Context, nam
708711
childCRB := &rbacv1.ClusterRoleBinding{}
709712
childCRB.Name = crbRef.Name
710713

711-
labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)
714+
labelMap := ManagedObjectLabelMap(r.syncGVK.Kind, rsRef)
712715
// Remove sync-name label since the ClusterRoleBinding may be shared
713716
delete(labelMap, metadata.SyncNameLabel)
714717

@@ -792,7 +795,7 @@ func (r *reconcilerBase) patchSyncMetadata(ctx context.Context, rs client.Object
792795
// Some other tooling previously claimed this RSync as an ApplySet parent.
793796
// According to the ApplySet KEP, we MUST error if the tooling name does not match.
794797
return false, fmt.Errorf("%s applyset owned by %s: remove the %s annotation to allow adoption",
795-
r.syncKind, currentApplySetToolingValue, metadata.ApplySetToolingAnnotation)
798+
r.syncGVK.Kind, currentApplySetToolingValue, metadata.ApplySetToolingAnnotation)
796799
}
797800
// Else, if the name is ours, we can adopt it and update the version.
798801
// In the future we may want some version migration behavior,
@@ -823,3 +826,62 @@ func (r *reconcilerBase) patchSyncMetadata(ctx context.Context, rs client.Object
823826
r.logger(ctx).Info("Sync metadata update successful")
824827
return true, nil
825828
}
829+
830+
func (r *reconcilerBase) requeueRSync(ctx context.Context, obj client.Object, rsRef types.NamespacedName) []reconcile.Request {
831+
r.logger(ctx).Info(fmt.Sprintf("Changes to %s triggered a reconciliation for the %s (%s).",
832+
kinds.ObjectSummary(obj), r.syncGVK.Kind, rsRef))
833+
return []reconcile.Request{
834+
{NamespacedName: rsRef},
835+
}
836+
}
837+
838+
func (r *reconcilerBase) requeueAllRSyncs(ctx context.Context, obj client.Object) []reconcile.Request {
839+
syncMetaList, err := r.listSyncMetadata(ctx)
840+
if err != nil {
841+
r.logger(ctx).Error(err, "Failed to list objects",
842+
logFieldSyncKind, r.syncGVK.Kind)
843+
return nil
844+
}
845+
requests := make([]reconcile.Request, len(syncMetaList.Items))
846+
for i, syncMeta := range syncMetaList.Items {
847+
requests[i] = reconcile.Request{
848+
NamespacedName: client.ObjectKeyFromObject(&syncMeta),
849+
}
850+
}
851+
if len(requests) > 0 {
852+
r.logger(ctx).Info(fmt.Sprintf("Changes to %s triggered reconciliations for %d %s objects.",
853+
kinds.ObjectSummary(obj), len(syncMetaList.Items), r.syncGVK.Kind))
854+
}
855+
return requests
856+
}
857+
858+
func (r *reconcilerBase) listSyncMetadata(ctx context.Context, opts ...client.ListOption) (*metav1.PartialObjectMetadataList, error) {
859+
syncMetaList := &metav1.PartialObjectMetadataList{}
860+
syncMetaList.SetGroupVersionKind(kinds.ListGVKForItemGVK(r.syncGVK))
861+
if err := r.client.List(ctx, syncMetaList, opts...); err != nil {
862+
return nil, err
863+
}
864+
return syncMetaList, nil
865+
}
866+
867+
// isAnnotationValueTrue returns whether the annotation should be enabled for the
868+
// reconciler of this RSync. This is determined by an annotation that is set on
869+
// the RSync by the reconciler.
870+
func (r *reconcilerBase) isAnnotationValueTrue(ctx context.Context, obj core.Annotated, key string) bool {
871+
annotations := obj.GetAnnotations()
872+
if annotations == nil {
873+
return false
874+
}
875+
val, ok := obj.GetAnnotations()[key]
876+
if !ok { // default to disabling the annotation
877+
return false
878+
}
879+
boolVal, err := strconv.ParseBool(val)
880+
if err != nil {
881+
// This should never happen, as the annotation should always be set to a
882+
// valid value by the reconciler. Log the error and return the default value.
883+
r.logger(ctx).Error(err, "Failed to parse annotation value as boolean: %s: %s", key, val)
884+
return false
885+
}
886+
return boolVal
887+
}

‎pkg/reconcilermanager/controllers/reposync_controller.go‎

Lines changed: 27 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func NewRepoSyncReconciler(clusterName string, reconcilerPollingPeriod, hydratio
106106
scheme: scheme,
107107
reconcilerPollingPeriod: reconcilerPollingPeriod,
108108
hydrationPollingPeriod: hydrationPollingPeriod,
109-
syncKind: configsync.RepoSyncKind,
109+
syncGVK: kinds.RepoSyncV1Beta1(),
110110
knownHostExist: false,
111111
controllerName: "",
112112
},
@@ -130,7 +130,7 @@ func (r *RepoSyncReconciler) Reconcile(ctx context.Context, req controllerruntim
130130
Name: core.NsReconcilerName(rsRef.Namespace, rsRef.Name),
131131
}
132132
ctx = r.setLoggerValues(ctx,
133-
logFieldSyncKind, r.syncKind,
133+
logFieldSyncKind, r.syncGVK.Kind,
134134
logFieldSyncRef, rsRef.String(),
135135
logFieldReconciler, reconcilerRef.String())
136136
rs := &v1beta1.RepoSync{}
@@ -220,7 +220,7 @@ func (r *RepoSyncReconciler) upsertManagedObjects(ctx context.Context, reconcile
220220
rsRef := client.ObjectKeyFromObject(rs)
221221
r.logger(ctx).V(3).Info("Reconciling managed objects")
222222

223-
labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)
223+
labelMap := ManagedObjectLabelMap(r.syncGVK.Kind, rsRef)
224224

225225
// Create secret in config-management-system namespace using the
226226
// existing secret in the reposync.namespace.
@@ -561,11 +561,14 @@ func (r *RepoSyncReconciler) watchConfigMaps(rs *v1beta1.RepoSync) error {
561561

562562
func (r *RepoSyncReconciler) mapMembershipToRepoSyncs(ctx context.Context, o client.Object) []reconcile.Request {
563563
// Clear the membership if the cluster is unregistered
564-
if err := r.client.Get(ctx, types.NamespacedName{Name: fleetMembershipName}, &hubv1.Membership{}); err != nil {
564+
membershipObj := &hubv1.Membership{}
565+
membershipObj.Name = fleetMembershipName
566+
membershipRef := client.ObjectKeyFromObject(membershipObj)
567+
if err := r.client.Get(ctx, membershipRef, membershipObj); err != nil {
565568
if apierrors.IsNotFound(err) {
566569
klog.Info("Fleet Membership not found, clearing membership cache")
567570
r.membership = nil
568-
return r.requeueAllRepoSyncs(fleetMembershipName)
571+
return r.requeueAllRSyncs(ctx, membershipObj)
569572
}
570573
klog.Errorf("Fleet Membership get failed: %v", err)
571574
return nil
@@ -581,28 +584,7 @@ func (r *RepoSyncReconciler) mapMembershipToRepoSyncs(ctx context.Context, o cli
581584
return nil
582585
}
583586
r.membership = m
584-
return r.requeueAllRepoSyncs(fleetMembershipName)
585-
}
586-
587-
func (r *RepoSyncReconciler) requeueAllRepoSyncs(name string) []reconcile.Request {
588-
//TODO: pass through context (reqs updating controller-runtime)
589-
ctx := context.Background()
590-
allRepoSyncs := &v1beta1.RepoSyncList{}
591-
if err := r.client.List(ctx, allRepoSyncs); err != nil {
592-
klog.Errorf("RepoSync list failed: %v", err)
593-
return nil
594-
}
595-
596-
requests := make([]reconcile.Request, len(allRepoSyncs.Items))
597-
for i, rs := range allRepoSyncs.Items {
598-
requests[i] = reconcile.Request{
599-
NamespacedName: client.ObjectKeyFromObject(&rs),
600-
}
601-
}
602-
if len(requests) > 0 {
603-
klog.Infof("Changes to %s trigger reconciliations for %d RepoSync objects.", name, len(allRepoSyncs.Items))
604-
}
605-
return requests
587+
return r.requeueAllRSyncs(ctx, membershipObj)
606588
}
607589

608590
// mapSecretToRepoSyncs define a mapping from the Secret object to its attached
@@ -633,7 +615,7 @@ func (r *RepoSyncReconciler) mapSecretToRepoSyncs(ctx context.Context, secret cl
633615
// so requeue the mapped RepoSync object and then return.
634616
reconcilerName := core.NsReconcilerName(rs.GetNamespace(), rs.GetName())
635617
if isUpsertedSecret(&rs, sRef.Name) {
636-
return requeueRepoSyncRequest(secret, client.ObjectKeyFromObject(&rs))
618+
return r.requeueRSync(ctx, secret, client.ObjectKeyFromObject(&rs))
637619
}
638620
isSAToken := strings.HasPrefix(sRef.Name, reconcilerName+"-token-")
639621
if isSAToken {
@@ -648,7 +630,7 @@ func (r *RepoSyncReconciler) mapSecretToRepoSyncs(ctx context.Context, secret cl
648630
}
649631
for _, s := range serviceAccount.Secrets {
650632
if s.Name == sRef.Name {
651-
return requeueRepoSyncRequest(secret, client.ObjectKeyFromObject(&rs))
633+
return r.requeueRSync(ctx, secret, client.ObjectKeyFromObject(&rs))
652634
}
653635
}
654636
}
@@ -686,9 +668,9 @@ func (r *RepoSyncReconciler) mapSecretToRepoSyncs(ctx context.Context, secret cl
686668
return requests
687669
}
688670

689-
func (r *RepoSyncReconciler) mapAdmissionWebhookToRepoSyncs(_ context.Context, admissionWebhook client.Object) []reconcile.Request {
671+
func (r *RepoSyncReconciler) mapAdmissionWebhookToRepoSyncs(ctx context.Context, admissionWebhook client.Object) []reconcile.Request {
690672
if admissionWebhook.GetName() == webhookconfiguration.Name {
691-
return r.requeueAllRepoSyncs(admissionWebhook.GetName())
673+
return r.requeueAllRSyncs(ctx, admissionWebhook)
692674
}
693675
return nil
694676
}
@@ -782,7 +764,7 @@ func (r *RepoSyncReconciler) mapConfigMapToRepoSyncs(ctx context.Context, obj cl
782764
}
783765
}
784766
if len(rsRef.Name) > 0 && len(rsRef.Namespace) > 0 {
785-
return requeueRepoSyncRequest(obj, rsRef)
767+
return r.requeueRSync(ctx, obj, rsRef)
786768
}
787769
return nil
788770
}
@@ -857,10 +839,10 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(ctx context.Context, obj client
857839
}
858840
}
859841

860-
allRepoSyncs := &v1beta1.RepoSyncList{}
861-
if err := r.client.List(ctx, allRepoSyncs); err != nil {
862-
klog.Errorf("failed to list all RepoSyncs for %s (%s): %v",
863-
obj.GetObjectKind().GroupVersionKind().Kind, objRef, err)
842+
syncMetaList, err := r.listSyncMetadata(ctx)
843+
if err != nil {
844+
r.logger(ctx).Error(err, "Failed to list objects",
845+
logFieldSyncKind, r.syncGVK.Kind)
864846
return nil
865847
}
866848

@@ -869,19 +851,19 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(ctx context.Context, obj client
869851
// For other resources, requeue the mapping RepoSync object and then return.
870852
var requests []reconcile.Request
871853
var attachedRSNames []string
872-
for _, rs := range allRepoSyncs.Items {
873-
reconcilerName := core.NsReconcilerName(rs.GetNamespace(), rs.GetName())
854+
for _, syncMeta := range syncMetaList.Items {
855+
reconcilerName := core.NsReconcilerName(syncMeta.GetNamespace(), syncMeta.GetName())
874856
switch obj.(type) {
875857
case *rbacv1.RoleBinding:
876-
if objRef.Name == nsRoleBindingName && objRef.Namespace == rs.Namespace {
858+
if objRef.Name == nsRoleBindingName && objRef.Namespace == syncMeta.Namespace {
877859
requests = append(requests, reconcile.Request{
878-
NamespacedName: client.ObjectKeyFromObject(&rs),
860+
NamespacedName: client.ObjectKeyFromObject(&syncMeta),
879861
})
880-
attachedRSNames = append(attachedRSNames, rs.GetName())
862+
attachedRSNames = append(attachedRSNames, syncMeta.GetName())
881863
}
882864
default: // Deployment and ServiceAccount
883865
if objRef.Name == reconcilerName {
884-
return requeueRepoSyncRequest(obj, client.ObjectKeyFromObject(&rs))
866+
return r.requeueRSync(ctx, obj, client.ObjectKeyFromObject(&syncMeta))
885867
}
886868
}
887869
}
@@ -892,16 +874,6 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(ctx context.Context, obj client
892874
return requests
893875
}
894876

895-
func requeueRepoSyncRequest(obj client.Object, rsRef types.NamespacedName) []reconcile.Request {
896-
klog.Infof("Changes to %s triggered a reconciliation for the RepoSync (%s).",
897-
kinds.ObjectSummary(obj), rsRef)
898-
return []reconcile.Request{
899-
{
900-
NamespacedName: rsRef,
901-
},
902-
}
903-
}
904-
905877
func (r *RepoSyncReconciler) populateContainerEnvs(ctx context.Context, rs *v1beta1.RepoSync, reconcilerName string) map[string][]corev1.EnvVar {
906878
result := map[string][]corev1.EnvVar{
907879
reconcilermanager.HydrationController: hydrationEnvs(hydrationOptions{
@@ -926,7 +898,7 @@ func (r *RepoSyncReconciler) populateContainerEnvs(ctx context.Context, rs *v1be
926898
statusMode: rs.Spec.SafeOverride().StatusMode,
927899
reconcileTimeout: v1beta1.GetReconcileTimeout(rs.Spec.SafeOverride().ReconcileTimeout),
928900
apiServerTimeout: v1beta1.GetAPIServerTimeout(rs.Spec.SafeOverride().APIServerTimeout),
929-
requiresRendering: annotationEnabled(metadata.RequiresRenderingAnnotationKey, rs.GetAnnotations()),
901+
requiresRendering: r.isAnnotationValueTrue(ctx, rs, metadata.RequiresRenderingAnnotationKey),
930902
// Namespace reconciler doesn't support NamespaceSelector at all.
931903
dynamicNSSelectorEnabled: false,
932904
webhookEnabled: r.webhookEnabled,
@@ -1090,7 +1062,7 @@ func (r *RepoSyncReconciler) upsertSharedRoleBinding(ctx context.Context, reconc
10901062
childRB := &rbacv1.RoleBinding{}
10911063
childRB.Name = rbRef.Name
10921064
childRB.Namespace = rbRef.Namespace
1093-
labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)
1065+
labelMap := ManagedObjectLabelMap(r.syncGVK.Kind, rsRef)
10941066
// Remove sync-name label since the RoleBinding may be shared
10951067
delete(labelMap, metadata.SyncNameLabel)
10961068

@@ -1242,7 +1214,7 @@ func (r *RepoSyncReconciler) mutationsFor(ctx context.Context, rs *v1beta1.RepoS
12421214
case reconcilermanager.Reconciler:
12431215
container.Env = append(container.Env, containerEnvs[container.Name]...)
12441216
case reconcilermanager.HydrationController:
1245-
if !annotationEnabled(metadata.RequiresRenderingAnnotationKey, rs.GetAnnotations()) {
1217+
if !r.isAnnotationValueTrue(ctx, rs, metadata.RequiresRenderingAnnotationKey) {
12461218
// if the sync source does not require rendering, omit the hydration controller
12471219
// this minimizes the resource footprint of the reconciler
12481220
addContainer = false

0 commit comments

Comments
 (0)