Skip to content

Commit

Permalink
fix: apply managed resource labels consistently (#1052)
Browse files Browse the repository at this point in the history 
This change updates the label logic for the resources managed by
reconciler-manager to apply a consistent set of rules.

- Apply the sync-kind and sync-namespace label for shared
  RBAC bindings, leaving out the sync-name field given that multiple
  RSyncs may be mapped to the same binding.
  • Loading branch information
@sdowell
sdowell authored Nov 21, 2023
1 parent 364d7d0 commit 2389d0b
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 35 deletions.
3 changes: 2 additions & 1 deletion pkg/reconcilermanager/controllers/reconciler_base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,14 +649,15 @@ func ManagedObjectLabelMap(syncKind string, rsRef types.NamespacedName) map[stri
}
}

func (r *reconcilerBase) updateRBACBinding(ctx context.Context, reconcilerRef types.NamespacedName, binding client.Object) error {
func (r *reconcilerBase) updateRBACBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName, binding client.Object) error {
existingBinding := binding.DeepCopyObject()
subjects := []rbacv1.Subject{r.serviceAccountSubject(reconcilerRef)}
if crb, ok := binding.(*rbacv1.ClusterRoleBinding); ok {
crb.Subjects = subjects
} else if rb, ok := binding.(*rbacv1.RoleBinding); ok {
rb.Subjects = subjects
}
core.AddLabels(binding, ManagedObjectLabelMap(r.syncKind, rsRef))
if equality.Semantic.DeepEqual(existingBinding, binding) {
return nil
}
Expand Down
16 changes: 8 additions & 8 deletions pkg/reconcilermanager/controllers/reposync_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,24 +207,20 @@ func (r *RepoSyncReconciler) upsertManagedObjects(ctx context.Context, reconcile
rsRef := client.ObjectKeyFromObject(rs)
r.logger(ctx).V(3).Info("Reconciling managed objects")

labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)

// Create secret in config-management-system namespace using the
// existing secret in the reposync.namespace.
if _, err := r.upsertAuthSecret(ctx, rs, reconcilerRef); err != nil {
if _, err := r.upsertAuthSecret(ctx, rs, reconcilerRef, labelMap); err != nil {
return errors.Wrap(err, "upserting auth secret")
}

// Create secret in config-management-system namespace using the
// existing secret in the reposync.namespace.
if _, err := r.upsertCACertSecret(ctx, rs, reconcilerRef); err != nil {
if _, err := r.upsertCACertSecret(ctx, rs, reconcilerRef, labelMap); err != nil {
return errors.Wrap(err, "upserting CA cert secret")
}

labelMap := map[string]string{
metadata.SyncNamespaceLabel: rs.Namespace,
metadata.SyncNameLabel: rs.Name,
metadata.SyncKindLabel: r.syncKind,
}

// Overwrite reconciler pod ServiceAccount.
var auth configsync.AuthType
var gcpSAEmail string
Expand Down Expand Up @@ -983,8 +979,12 @@ func (r *RepoSyncReconciler) upsertSharedRoleBinding(ctx context.Context, reconc
childRB := &rbacv1.RoleBinding{}
childRB.Name = rbRef.Name
childRB.Namespace = rbRef.Namespace
labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)
// Remove sync-name label since the RoleBinding may be shared
delete(labelMap, metadata.SyncNameLabel)

op, err := CreateOrUpdate(ctx, r.client, childRB, func() error {
core.AddLabels(childRB, labelMap)
childRB.RoleRef = rolereference(RepoSyncBaseClusterRoleName, "ClusterRole")
childRB.Subjects = addSubject(childRB.Subjects, r.serviceAccountSubject(reconcilerRef))
return nil
Expand Down
13 changes: 9 additions & 4 deletions pkg/reconcilermanager/controllers/rootsync_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ func (r *RootSyncReconciler) manageRBACBindings(ctx context.Context, reconcilerR
}
if len(roleRefs) == 0 {
// Backwards compatible behavior to default to cluster-admin
if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncLegacyClusterRoleBindingName, "cluster-admin", reconcilerRef); err != nil {
if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncLegacyClusterRoleBindingName, "cluster-admin", reconcilerRef, rsRef); err != nil {
return err
}
// Clean up any RoleRefs created previously that are no longer declared
Expand All @@ -914,7 +914,7 @@ func (r *RootSyncReconciler) manageRBACBindings(ctx context.Context, reconcilerR
return nil
}
// Add the base ClusterRole for basic root reconciler functionality
if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncBaseClusterRoleBindingName, RootSyncBaseClusterRoleName, reconcilerRef); err != nil {
if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncBaseClusterRoleBindingName, RootSyncBaseClusterRoleName, reconcilerRef, rsRef); err != nil {
return err
}
declaredRoleRefs := roleRefs
Expand All @@ -937,7 +937,7 @@ func (r *RootSyncReconciler) manageRBACBindings(ctx context.Context, reconcilerR
// - if they are no longer declared in roleRefs, delete
for roleRef, binding := range currentRefMap {
if _, ok := declaredRefMap[roleRef]; ok { // update
if err := r.updateRBACBinding(ctx, reconcilerRef, binding); err != nil {
if err := r.updateRBACBinding(ctx, reconcilerRef, rsRef, binding); err != nil {
return errors.Wrap(err, "upserting RBAC Binding")
}
} else { // Clean up any RoleRefs created previously that are no longer declared
Expand All @@ -956,12 +956,17 @@ func (r *RootSyncReconciler) manageRBACBindings(ctx context.Context, reconcilerR
return nil
}

func (r *RootSyncReconciler) upsertSharedClusterRoleBinding(ctx context.Context, name, clusterRole string, reconcilerRef types.NamespacedName) error {
func (r *RootSyncReconciler) upsertSharedClusterRoleBinding(ctx context.Context, name, clusterRole string, reconcilerRef, rsRef types.NamespacedName) error {
crbRef := client.ObjectKey{Name: name}
childCRB := &rbacv1.ClusterRoleBinding{}
childCRB.Name = crbRef.Name

labelMap := ManagedObjectLabelMap(r.syncKind, rsRef)
// Remove sync-name label since the ClusterRoleBinding may be shared
delete(labelMap, metadata.SyncNameLabel)

op, err := CreateOrUpdate(ctx, r.client, childCRB, func() error {
core.AddLabels(childCRB, labelMap)
childCRB.OwnerReferences = nil
childCRB.RoleRef = rolereference(clusterRole, "ClusterRole")
childCRB.Subjects = addSubject(childCRB.Subjects, r.serviceAccountSubject(reconcilerRef))
Expand Down
31 changes: 21 additions & 10 deletions pkg/reconcilermanager/controllers/rootsync_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1579,11 +1579,10 @@ func validateReconcilerClusterRoleBindings(fakeClient *syncerFake.Client, rootSy

opts := &client.ListOptions{}
opts.LabelSelector = client.MatchingLabelsSelector{
Selector: labels.SelectorFromSet(map[string]string{
metadata.SyncKindLabel: configsync.RootSyncKind,
metadata.SyncNameLabel: rootSyncName,
metadata.SyncNamespaceLabel: configsync.ControllerNamespace,
}),
Selector: labels.SelectorFromSet(ManagedObjectLabelMap(
configsync.RootSyncKind,
types.NamespacedName{Name: rootSyncName, Namespace: configsync.ControllerNamespace},
)),
}
err := fakeClient.List(ctx, got, opts)
if err != nil {
Expand Down Expand Up @@ -1619,11 +1618,7 @@ func validateReconcilerRoleBindings(fakeClient *syncerFake.Client, syncKind stri

opts := &client.ListOptions{}
opts.LabelSelector = client.MatchingLabelsSelector{
Selector: labels.SelectorFromSet(map[string]string{
metadata.SyncKindLabel: syncKind,
metadata.SyncNameLabel: rsRef.Name,
metadata.SyncNamespaceLabel: rsRef.Namespace,
}),
Selector: labels.SelectorFromSet(ManagedObjectLabelMap(syncKind, rsRef)),
}
err := fakeClient.List(ctx, got, opts)
if err != nil {
Expand Down Expand Up @@ -1710,6 +1705,10 @@ func TestRootSyncCreateWithOverrideRoleRefs(t *testing.T) {
RootSyncBaseClusterRoleBindingName,
RootSyncBaseClusterRoleName,
core.UID("1"), core.ResourceVersion("1"), core.Generation(1),
core.Labels(map[string]string{
metadata.SyncKindLabel: configsync.RootSyncKind,
metadata.SyncNamespaceLabel: configsync.ControllerNamespace,
}),
)
defaultCrb.Subjects = addSubjectByName(nil, rootReconcilerName)

Expand Down Expand Up @@ -1745,6 +1744,10 @@ func TestRootSyncCreateWithOverrideRoleRefs(t *testing.T) {
RootSyncLegacyClusterRoleBindingName,
"cluster-admin",
core.UID("1"), core.ResourceVersion("1"), core.Generation(1),
core.Labels(map[string]string{
metadata.SyncKindLabel: configsync.RootSyncKind,
metadata.SyncNamespaceLabel: configsync.ControllerNamespace,
}),
)
clusterAdminCRB.Subjects = addSubjectByName(nil, rootReconcilerName)
if err := validateClusterRoleBinding(clusterAdminCRB, fakeClient); err != nil {
Expand All @@ -1770,6 +1773,10 @@ func TestMigrationToIndividualClusterRoleBindingsWhenDefaultRootSyncExists(t *te
RootSyncLegacyClusterRoleBindingName,
"cluster-admin",
core.UID("1"), core.ResourceVersion("1"), core.Generation(1),
core.Labels(map[string]string{
metadata.SyncKindLabel: configsync.RootSyncKind,
metadata.SyncNamespaceLabel: configsync.ControllerNamespace,
}),
)
oldBinding.Subjects = addSubjectByName(nil, rs1ReconcilerName)
oldBinding.Subjects = addSubjectByName(oldBinding.Subjects, rs2ReconcilerName)
Expand Down Expand Up @@ -2049,6 +2056,10 @@ func TestMultipleRootSyncs(t *testing.T) {
RootSyncLegacyClusterRoleBindingName,
"cluster-admin",
core.UID("1"), core.ResourceVersion("1"), core.Generation(1),
core.Labels(map[string]string{
metadata.SyncKindLabel: configsync.RootSyncKind,
metadata.SyncNamespaceLabel: configsync.ControllerNamespace,
}),
)
crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName)
rootContainerEnv1 := testReconciler.populateContainerEnvs(ctx, rs1, rootReconcilerName)
Expand Down
18 changes: 7 additions & 11 deletions pkg/reconcilermanager/controllers/secret.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"kpt.dev/configsync/pkg/api/configsync"
"kpt.dev/configsync/pkg/api/configsync/v1beta1"
"kpt.dev/configsync/pkg/core"
"kpt.dev/configsync/pkg/metadata"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)
Expand Down Expand Up @@ -60,7 +59,7 @@ func shouldUpsertHelmSecret(rs *v1beta1.RepoSync) bool {
// upsertAuthSecret creates or updates the auth secret in the
// config-management-system namespace using an existing secret in the RepoSync
// namespace.
func (r *reconcilerBase) upsertAuthSecret(ctx context.Context, rs *v1beta1.RepoSync, reconcilerRef types.NamespacedName) (client.ObjectKey, error) {
func (r *reconcilerBase) upsertAuthSecret(ctx context.Context, rs *v1beta1.RepoSync, reconcilerRef types.NamespacedName, labelMap map[string]string) (client.ObjectKey, error) {
rsRef := client.ObjectKeyFromObject(rs)
switch {
case shouldUpsertGitSecret(rs):
Expand All @@ -69,15 +68,15 @@ func (r *reconcilerBase) upsertAuthSecret(ctx context.Context, rs *v1beta1.RepoS
if err != nil {
return cmsSecretRef, errors.Wrap(err, "user secret required for git client authentication")
}
_, err = r.upsertSecret(ctx, cmsSecretRef, rsRef, userSecret)
_, err = r.upsertSecret(ctx, cmsSecretRef, userSecret, labelMap)
return cmsSecretRef, err
case shouldUpsertHelmSecret(rs):
nsSecretRef, cmsSecretRef := getSecretRefs(rsRef, reconcilerRef, v1beta1.GetSecretName(rs.Spec.Helm.SecretRef))
userSecret, err := getUserSecret(ctx, r.client, nsSecretRef)
if err != nil {
return cmsSecretRef, errors.Wrap(err, "user secret required for helm client authentication")
}
_, err = r.upsertSecret(ctx, cmsSecretRef, rsRef, userSecret)
_, err = r.upsertSecret(ctx, cmsSecretRef, userSecret, labelMap)
return cmsSecretRef, err
default:
// No secret required
Expand All @@ -88,15 +87,15 @@ func (r *reconcilerBase) upsertAuthSecret(ctx context.Context, rs *v1beta1.RepoS
// upsertCACertSecret creates or updates the CA cert secret in the
// config-management-system namespace using an existing secret in the RepoSync
// namespace.
func (r *reconcilerBase) upsertCACertSecret(ctx context.Context, rs *v1beta1.RepoSync, reconcilerRef types.NamespacedName) (client.ObjectKey, error) {
func (r *reconcilerBase) upsertCACertSecret(ctx context.Context, rs *v1beta1.RepoSync, reconcilerRef types.NamespacedName, labelMap map[string]string) (client.ObjectKey, error) {
rsRef := client.ObjectKeyFromObject(rs)
if shouldUpsertCACertSecret(rs) {
nsSecretRef, cmsSecretRef := getSecretRefs(rsRef, reconcilerRef, v1beta1.GetSecretName(rs.Spec.Git.CACertSecretRef))
userSecret, err := getUserSecret(ctx, r.client, nsSecretRef)
if err != nil {
return cmsSecretRef, errors.Wrap(err, "user secret required for git server validation")
}
_, err = r.upsertSecret(ctx, cmsSecretRef, rsRef, userSecret)
_, err = r.upsertSecret(ctx, cmsSecretRef, userSecret, labelMap)
return cmsSecretRef, err
}
// No secret required
Expand Down Expand Up @@ -133,16 +132,13 @@ func getUserSecret(ctx context.Context, c client.Client, nsSecretRef client.Obje

// upsertSecret creates or updates a secret in config-management-system
// namespace using an existing user secret.
func (r *reconcilerBase) upsertSecret(ctx context.Context, cmsSecretRef, rsRef types.NamespacedName, userSecret *corev1.Secret) (controllerutil.OperationResult, error) {
func (r *reconcilerBase) upsertSecret(ctx context.Context, cmsSecretRef types.NamespacedName, userSecret *corev1.Secret, labelMap map[string]string) (controllerutil.OperationResult, error) {
cmsSecret := &corev1.Secret{}
cmsSecret.Name = cmsSecretRef.Name
cmsSecret.Namespace = cmsSecretRef.Namespace

op, err := CreateOrUpdate(ctx, r.client, cmsSecret, func() error {
core.AddLabels(cmsSecret, map[string]string{
metadata.SyncNamespaceLabel: rsRef.Namespace,
metadata.SyncNameLabel: rsRef.Name,
})
core.AddLabels(cmsSecret, labelMap)
// Copy user secret data & type to managed secret
cmsSecret.Data = userSecret.Data
cmsSecret.Type = userSecret.Type
Expand Down
7 changes: 6 additions & 1 deletion pkg/reconcilermanager/controllers/secret_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ func secret(t *testing.T, name, data string, auth configsync.AuthType, sourceTyp
result.SetLabels(map[string]string{
metadata.SyncNamespaceLabel: reposyncNs,
metadata.SyncNameLabel: reposyncName,
metadata.SyncKindLabel: configsync.RepoSyncKind,
})
return result
}
Expand Down Expand Up @@ -197,7 +198,11 @@ func TestUpsertAuthSecret(t *testing.T) {
},
client: tc.client,
}
sKey, err := r.upsertAuthSecret(ctx, tc.reposync, nsReconcilerKey)
labelMap := ManagedObjectLabelMap(configsync.RepoSyncKind, types.NamespacedName{
Name: tc.reposync.Name,
Namespace: tc.reposync.Namespace,
})
sKey, err := r.upsertAuthSecret(ctx, tc.reposync, nsReconcilerKey, labelMap)
assert.Equal(t, tc.wantKey, sKey, "unexpected secret key returned")
if tc.wantError {
assert.Error(t, err, "expected upsertAuthSecret to error")
Expand Down

0 comments on commit 2389d0b

Please sign in to comment.