From add8cc03f588e0ebedf58cd0e890355cd9ccf4d1 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 16 Sep 2022 14:54:12 -0400 Subject: [PATCH 01/21] update scopeinstance_controller.go Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 451 +++++++++++++++--------- 1 file changed, 281 insertions(+), 170 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index fd6b32f..a503204 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -25,12 +25,17 @@ import ( "awgreene/scope-operator/util" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" + apimacherrors "k8s.io/apimachinery/pkg/util/errors" + metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" + rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -77,11 +82,37 @@ func (r *ScopeInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Reques log.Log.Info("Reconciling ScopeInstance", "namespaceName", req.NamespacedName) - in := &operatorsv1.ScopeInstance{} - if err := r.Client.Get(ctx, req.NamespacedName, in); err != nil { - return ctrl.Result{}, err + existingIn := &operatorsv1.ScopeInstance{} + if err := r.Client.Get(ctx, req.NamespacedName, existingIn); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) } + // Perform reconciliation + reconciledIn := existingIn.DeepCopy() + res, reconcileErr := r.reconcile(ctx, reconciledIn) + + // Update the status subresource before updating the main object. This is + // necessary because, in many cases, the main object update will remove the + // finalizer, which will cause the core Kubernetes deletion logic to + // complete. Therefore, we need to make the status update prior to the main + // object update to ensure that the status update can be processed before + // a potential deletion. + if !equality.Semantic.DeepEqual(existingIn.Status, reconciledIn.Status) { + if updateErr := r.Client.Status().Update(ctx, reconciledIn); updateErr != nil { + return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + } + } + existingIn.Status, reconciledIn.Status = operatorsv1.ScopeInstanceStatus{}, operatorsv1.ScopeInstanceStatus{} + if !equality.Semantic.DeepEqual(existingIn, reconciledIn) { + if updateErr := r.Client.Update(ctx, reconciledIn); updateErr != nil { + return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + } + } + return res, reconcileErr +} + +func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1.ScopeInstance) (ctrl.Result, error) { + // Get the ScopeTemplate referenced by the ScopeInstance st := &operatorsv1.ScopeTemplate{} if err := r.Client.Get(ctx, client.ObjectKey{Name: in.Spec.ScopeTemplateName}, st); err != nil { if !k8sapierrors.IsNotFound(err) { @@ -94,7 +125,7 @@ func (r *ScopeInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Reques } if err := r.deleteBindings(ctx, listOption); err != nil { - log.Log.Error(err, "in deleting Role Bindings") + log.Log.Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } @@ -103,208 +134,197 @@ func (r *ScopeInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Reques // create required roleBindings and clusterRoleBindings. if err := r.ensureBindings(ctx, in, st); err != nil { - log.Log.Error(err, "in creating Role Bindings") + log.Log.Error(err, "in creating RoleBindings") return ctrl.Result{}, err } - listOption := client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - } - - requirement, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) - if err != nil { + // delete out of date (Cluster)RoleBindings + if err := r.deleteOldBindings(ctx, in, st); err != nil { + log.Log.Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } - listOptions := &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), + return ctrl.Result{}, nil +} + +func (r *ScopeInstanceReconciler) ensureBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { + // it will create clusterrole as shown below if no namespace is provided + for _, cr := range st.Spec.ClusterRoles { + if len(in.Spec.Namespaces) == 0 { + err := r.createOrUpdateClusterRoleBinding(ctx, &cr, in, st) + if err != nil { + return err + } + } else { + for _, ns := range in.Spec.Namespaces { + err := r.createOrUpdateRoleBinding(ctx, &cr, in, st, ns) + if err != nil { + return err + } + } + } } - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { - log.Log.Error(err, "in deleting Role Bindings") - return ctrl.Result{}, err + return nil +} + +func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { + crb := r.getClusterRoleBinding(cr, in, st) + crbList := &rbacv1.ClusterRoleBindingList{} + if err := r.Client.List(ctx, crbList, client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + scopeTemplateUIDKey: string(st.GetUID()), + clusterRoleBindingGenerateKey: cr.GenerateName, + }); err != nil { + return err } - // TODO: Find out how to merge with the above delete - listOption = client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), + if len(crbList.Items) > 1 { + return fmt.Errorf("more than one ClusterRoleBinding found for ClusterRole %s", cr.GenerateName) } - requirement, err = labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) - if err != nil { - return ctrl.Result{}, err + // GenerateName is immutable, so create the object if it has changed + if len(crbList.Items) == 0 { + if err := r.Client.Create(ctx, crb); err != nil { + return err + } + return nil } - listOptions = &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), + existingCRB := &crbList.Items[0] + if util.IsOwnedByLabel(existingCRB.DeepCopy(), in) && + reflect.DeepEqual(existingCRB.Subjects, crb.Subjects) && + reflect.DeepEqual(existingCRB.Labels, crb.Labels) { + log.Log.Info("existing ClusterRoleBinding does not need to be updated") + return nil } - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { - log.Log.Error(err, "in deleting Role Bindings") - return ctrl.Result{}, err + u, err := r.patchConfigForClusterRoleBinding(existingCRB, crb) + if err != nil { + return err } - log.Log.Info("No ScopeInstance error") + // server-side apply patch + if err := r.patchBinding(ctx, u); err != nil { + return err + } - return ctrl.Result{}, nil + return nil } -func (r *ScopeInstanceReconciler) ensureBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - // it will create clusterrole as shown below if no namespace is provided - // TODO: refactor code to handle both roleBindings and clusterRoleBindings - if len(in.Spec.Namespaces) == 0 { - for _, cr := range st.Spec.ClusterRoles { - crb := &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: cr.GenerateName + "-", - Labels: map[string]string{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - scopeInstanceHashKey: util.HashObject(in.Spec), - scopeTemplateHashKey: util.HashObject(st.Spec), - clusterRoleBindingGenerateKey: cr.GenerateName, - }, - OwnerReferences: []metav1.OwnerReference{{ - APIVersion: in.APIVersion, - Kind: in.Kind, - Name: in.GetObjectMeta().GetName(), - UID: in.GetObjectMeta().GetUID(), - }}, - }, - Subjects: cr.Subjects, - RoleRef: rbacv1.RoleRef{ - Kind: "ClusterRole", - Name: cr.GenerateName, - APIGroup: "rbac.authorization.k8s.io", - }, - } +func (r *ScopeInstanceReconciler) patchConfigForClusterRoleBinding(oldCrb *rbacv1.ClusterRoleBinding, crb *rbacv1.ClusterRoleBinding) (*unstructured.Unstructured, error) { + crbAc := rbacv1ac.ClusterRoleBinding(oldCrb.Name).WithLabels(crb.Labels) + subjAcs := []rbacv1ac.SubjectApplyConfiguration{} + orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} + for _, sub := range crb.Subjects { + subjAc := *rbacv1ac.Subject().WithAPIGroup(sub.APIGroup).WithKind(sub.Kind).WithName(sub.Name) + if sub.Namespace != "" { + subjAc.Namespace = &sub.Namespace + } - crbList := &rbacv1.ClusterRoleBindingList{} - if err := r.Client.List(ctx, crbList, client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - clusterRoleBindingGenerateKey: cr.GenerateName, - }); err != nil { - return err - } + subjAcs = append(subjAcs, subjAc) + } + for _, own := range crb.OwnerReferences { + ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) + orAcs = append(orAcs, ownAc) + } - if len(crbList.Items) > 1 { - return fmt.Errorf("more than one ClusterRoleBinding found for ClusterRole %s", cr.GenerateName) - } + crbAc.OwnerReferences = orAcs + crbAc.Subjects = subjAcs + crbAc.RoleRef = rbacv1ac.RoleRef().WithAPIGroup(crb.RoleRef.APIGroup).WithKind(crb.RoleRef.Kind).WithName(crb.RoleRef.Name) - // GenerateName is immutable, so create the object if it has changed - if len(crbList.Items) == 0 { - if err := r.Client.Create(ctx, crb); err != nil { - return err - } - continue - } + uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(crbAc) + if err != nil { + return nil, err + } - existingCRB := &crbList.Items[0] + return &unstructured.Unstructured{Object: uMap}, nil +} - if util.IsOwnedByLabel(existingCRB.DeepCopy(), in) && - reflect.DeepEqual(existingCRB.Subjects, crb.Subjects) && - reflect.DeepEqual(existingCRB.Labels, crb.Labels) { - log.Log.Info("existing ClusterRoleBinding does not need to be updated") - return nil - } - existingCRB.Labels = crb.Labels - existingCRB.OwnerReferences = crb.OwnerReferences - existingCRB.Subjects = crb.Subjects - - // server-side apply patch - existingCRB.ManagedFields = nil - if err := r.Client.Patch(ctx, - existingCRB, - client.Apply, - client.FieldOwner(siCtrlFieldOwner), - client.ForceOwnership); err != nil { - return err - } +func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) error { + rb := r.getRoleBinding(cr, in, st, namespace) + rbList := &rbacv1.RoleBindingList{} + if err := r.Client.List(ctx, rbList, &client.ListOptions{ + Namespace: namespace, + }, client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + scopeTemplateUIDKey: string(st.GetUID()), + clusterRoleBindingGenerateKey: cr.GenerateName, + }); err != nil { + return err + } + + if len(rbList.Items) > 1 { + return fmt.Errorf("more than one RoleBinding found for ClusterRole %s", cr.GenerateName) + } + // GenerateName is immutable, so create the object if it has changed + if len(rbList.Items) == 0 { + if err := r.Client.Create(ctx, rb); err != nil { + return err } - } else { - // it will iterate over the namespace and createrole bindings for each cluster roles - for _, namespace := range in.Spec.Namespaces { - for _, cr := range st.Spec.ClusterRoles { - rb := &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: cr.GenerateName + "-", - Namespace: namespace, - Labels: map[string]string{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - scopeInstanceHashKey: util.HashObject(in.Spec), - scopeTemplateHashKey: util.HashObject(st.Spec), - clusterRoleBindingGenerateKey: cr.GenerateName, - }, - OwnerReferences: []metav1.OwnerReference{{ - APIVersion: in.APIVersion, - Kind: in.Kind, - Name: in.GetObjectMeta().GetName(), - UID: in.GetObjectMeta().GetUID(), - }}, - }, - Subjects: cr.Subjects, - RoleRef: rbacv1.RoleRef{ - Kind: "ClusterRole", - Name: cr.GenerateName, - APIGroup: "rbac.authorization.k8s.io", - }, - } + return nil + } - rbList := &rbacv1.RoleBindingList{} - if err := r.Client.List(ctx, rbList, &client.ListOptions{ - Namespace: namespace, - }, client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - clusterRoleBindingGenerateKey: cr.GenerateName, - }); err != nil { - return err - } + log.Log.Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) - if len(rbList.Items) > 1 { - return fmt.Errorf("more than one roleBinding found for ClusterRole %s", cr.GenerateName) - } + existingRB := &rbList.Items[0] - // GenerateName is immutable, so create the object if it has changed - if len(rbList.Items) == 0 { - if err := r.Client.Create(ctx, rb); err != nil { - return err - } - continue - } + if util.IsOwnedByLabel(existingRB.DeepCopy(), in) && + reflect.DeepEqual(existingRB.Subjects, rb.Subjects) && + reflect.DeepEqual(existingRB.Labels, rb.Labels) { + log.Log.Info("existing RoleBinding does not need to be updated") + return nil + } - log.Log.Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) + u, err := r.patchConfigForRoleBinding(existingRB, rb) + if err != nil { + return err + } - existingRB := &rbList.Items[0] + // server-side apply patch + if err := r.patchBinding(ctx, u); err != nil { + return err + } - if util.IsOwnedByLabel(existingRB.DeepCopy(), in) && - reflect.DeepEqual(existingRB.Subjects, rb.Subjects) && - reflect.DeepEqual(existingRB.Labels, rb.Labels) { - log.Log.Info("existing ClusterRoleBinding does not need to be updated") - return nil - } - existingRB.Labels = rb.Labels - existingRB.OwnerReferences = rb.OwnerReferences - existingRB.Subjects = rb.Subjects - - // server-side apply patch - existingRB.ManagedFields = nil - if err := r.Client.Patch(ctx, - existingRB, - client.Apply, - client.FieldOwner(siCtrlFieldOwner), - client.ForceOwnership); err != nil { - return err - } - } + return nil +} + +func (r *ScopeInstanceReconciler) patchConfigForRoleBinding(oldRb *rbacv1.RoleBinding, rb *rbacv1.RoleBinding) (*unstructured.Unstructured, error) { + rbAc := rbacv1ac.ClusterRoleBinding(oldRb.Name).WithLabels(rb.Labels) + subjAcs := []rbacv1ac.SubjectApplyConfiguration{} + orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} + for _, sub := range rb.Subjects { + subjAc := *rbacv1ac.Subject().WithAPIGroup(sub.APIGroup).WithKind(sub.Kind).WithName(sub.Name) + if sub.Namespace != "" { + subjAc.Namespace = &sub.Namespace } + + subjAcs = append(subjAcs, subjAc) + } + for _, own := range rb.OwnerReferences { + ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) + orAcs = append(orAcs, ownAc) } - return nil + rbAc.OwnerReferences = orAcs + rbAc.Subjects = subjAcs + rbAc.RoleRef = rbacv1ac.RoleRef().WithAPIGroup(rb.RoleRef.APIGroup).WithKind(rb.RoleRef.Kind).WithName(rb.RoleRef.Name) + + uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rbAc) + if err != nil { + return nil, err + } + + return &unstructured.Unstructured{Object: uMap}, nil +} + +func (r *ScopeInstanceReconciler) patchBinding(ctx context.Context, binding client.Object) error { + return r.Client.Patch(ctx, + binding, + client.Apply, + client.FieldOwner(siCtrlFieldOwner), + client.ForceOwnership) } // TODO: use a client.DeleteAllOf instead of a client.List -> delete @@ -338,6 +358,46 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption return nil } +// deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. +func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { + listOption := client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + } + + requirement, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) + if err != nil { + return err + } + + listOptions := &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(*requirement), + } + + if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { + return err + } + + listOption = client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + scopeTemplateUIDKey: string(st.GetUID()), + } + + requirement, err = labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) + if err != nil { + return err + } + + listOptions = &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(*requirement), + } + + if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { + return err + } + + return nil +} + // SetupWithManager sets up the controller with the Manager. func (r *ScopeInstanceReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). @@ -368,9 +428,60 @@ func (r *ScopeInstanceReconciler) mapToScopeInstance(obj client.Object) (request request := reconcile.Request{ NamespacedName: types.NamespacedName{Namespace: si.GetNamespace(), Name: si.GetName()}, } - requests = append(requests, request) } return } + +// getClusterRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate, ScopeInstance, and ScopeTemplate +func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRoleBinding { + crb := &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: cr.GenerateName + "-", + Labels: map[string]string{ + scopeInstanceUIDKey: string(in.GetUID()), + scopeTemplateUIDKey: string(st.GetUID()), + scopeInstanceHashKey: util.HashObject(in.Spec), + scopeTemplateHashKey: util.HashObject(st.Spec), + clusterRoleBindingGenerateKey: cr.GenerateName, + }, + }, + Subjects: cr.Subjects, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: cr.GenerateName, + APIGroup: rbacv1.GroupName, + }, + } + + ctrl.SetControllerReference(in, crb, r.Scheme) + return crb +} + +// getRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate +func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) *rbacv1.RoleBinding { + rb := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: cr.GenerateName + "-", + Namespace: namespace, + Labels: map[string]string{ + scopeInstanceUIDKey: string(in.GetUID()), + scopeTemplateUIDKey: string(st.GetUID()), + scopeInstanceHashKey: util.HashObject(in.Spec), + scopeTemplateHashKey: util.HashObject(st.Spec), + clusterRoleBindingGenerateKey: cr.GenerateName, + }, + }, + Subjects: cr.Subjects, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: cr.GenerateName, + APIGroup: rbacv1.GroupName, + }, + } + + ctrl.SetControllerReference(in, rb, r.Scheme) + return rb + +} From 4a9f89e6a1648354c8fbfd9e8ce74dce4763a2e2 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 16 Sep 2022 15:52:22 -0400 Subject: [PATCH 02/21] update list options for delete in scopeinstance controller Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index a503204..d4fe6ef 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -360,38 +360,39 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption // deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - listOption := client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), + siHashReq, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) + if err != nil { + return err } - requirement, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) + siUIDReq, err := labels.NewRequirement(scopeInstanceUIDKey, selection.Equals, []string{string(in.GetUID())}) if err != nil { return err } listOptions := &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), + LabelSelector: labels.NewSelector().Add(*siHashReq, *siUIDReq), } - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { + if err := r.deleteBindings(ctx, listOptions); err != nil { return err } - listOption = client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), + stHashReq, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) + if err != nil { + return err } - requirement, err = labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) + stUIDReq, err := labels.NewRequirement(scopeTemplateUIDKey, selection.Equals, []string{string(st.GetUID())}) if err != nil { return err } listOptions = &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), + LabelSelector: labels.NewSelector().Add(*siUIDReq, *stUIDReq, *stHashReq), } - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { + if err := r.deleteBindings(ctx, listOptions); err != nil { return err } From 18f37103bfdd6d75dd12d53c1cb7e0592c52577c Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 16 Sep 2022 15:52:50 -0400 Subject: [PATCH 03/21] update controllers/scopetemplate_controller.go Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller.go | 167 ++++++++++++++++-------- 1 file changed, 113 insertions(+), 54 deletions(-) diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index e51e3bc..6d224d3 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -25,12 +25,17 @@ import ( "awgreene/scope-operator/util" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/equality" k8sapierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" + apimacherrors "k8s.io/apimachinery/pkg/util/errors" + metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" + rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -70,21 +75,39 @@ func (r *ScopeTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Reques log.Log.Info("Reconciling ScopeTemplate") // get the scope template - st := &operatorsv1.ScopeTemplate{} - if err := r.Client.Get(ctx, req.NamespacedName, st); err != nil { + existingSt := &operatorsv1.ScopeTemplate{} + if err := r.Client.Get(ctx, req.NamespacedName, existingSt); err != nil { return ctrl.Result{}, err } - scopeinstances := operatorsv1.ScopeInstanceList{} - - if err := r.Client.List(ctx, &scopeinstances, client.InNamespace(st.Namespace)); err != nil { - return ctrl.Result{}, err + // Perform reconciliation + reconciledSt := existingSt.DeepCopy() + res, reconcileErr := r.reconcile(ctx, reconciledSt) + + // Update the status subresource before updating the main object. This is + // necessary because, in many cases, the main object update will remove the + // finalizer, which will cause the core Kubernetes deletion logic to + // complete. Therefore, we need to make the status update prior to the main + // object update to ensure that the status update can be processed before + // a potential deletion. + if !equality.Semantic.DeepEqual(existingSt.Status, reconciledSt.Status) { + if updateErr := r.Client.Status().Update(ctx, reconciledSt); updateErr != nil { + return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + } + } + existingSt.Status, reconciledSt.Status = operatorsv1.ScopeTemplateStatus{}, operatorsv1.ScopeTemplateStatus{} + if !equality.Semantic.DeepEqual(existingSt, reconciledSt) { + if updateErr := r.Client.Update(ctx, reconciledSt); updateErr != nil { + return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + } } + return res, reconcileErr +} - listOptions := []client.ListOption{ - &client.MatchingLabels{ - scopeTemplateUIDKey: string(st.GetUID()), - }, +func (r *ScopeTemplateReconciler) reconcile(ctx context.Context, st *operatorsv1.ScopeTemplate) (ctrl.Result, error) { + scopeinstances := operatorsv1.ScopeInstanceList{} + if err := r.Client.List(ctx, &scopeinstances, &client.ListOptions{}); err != nil { + return ctrl.Result{}, err } for _, sInstance := range scopeinstances.Items { @@ -94,26 +117,31 @@ func (r *ScopeTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Reques // create ClusterRoles based on the ScopeTemplate log.Log.Info("ScopeInstance found that references ScopeTemplate", "name", st.Name) if err := r.ensureClusterRoles(ctx, st); err != nil { - return ctrl.Result{}, fmt.Errorf("Error in create ClusterRoles: %v", err) + return ctrl.Result{}, fmt.Errorf("creating ClusterRoles: %v", err) } + } - // Add requirement to delete old hashes - requirement, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) - if err != nil { - return ctrl.Result{}, err - } - listOptions = append(listOptions, &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), - }) - break + // Add requirement to delete old hashes + stHashReq, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) + if err != nil { + return ctrl.Result{}, err } - if err := r.deleteClusterRoles(ctx, listOptions...); err != nil { + // Only look for old clusterroles that map to this ScopeTemplate UID + stUIDReq, err := labels.NewRequirement(scopeTemplateUIDKey, selection.Equals, []string{string(st.GetUID())}) + if err != nil { return ctrl.Result{}, err } - log.Log.Info("No ScopeTemplate error") + listOptions := &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(*stHashReq, *stUIDReq), + } + if err := r.deleteClusterRoles(ctx, listOptions); err != nil { + return ctrl.Result{}, err + } + + log.Log.Info("No ScopeTemplate error") return ctrl.Result{}, nil } @@ -147,7 +175,6 @@ func (r *ScopeTemplateReconciler) mapToScopeTemplate(obj client.Object) (request request := reconcile.Request{ NamespacedName: types.NamespacedName{Namespace: obj.GetNamespace(), Name: scopeInstance.Spec.ScopeTemplateName}, } - requests = append(requests, request) return requests @@ -155,26 +182,10 @@ func (r *ScopeTemplateReconciler) mapToScopeTemplate(obj client.Object) (request func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *operatorsv1.ScopeTemplate) error { for _, cr := range st.Spec.ClusterRoles { - clusterRole := &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: cr.GenerateName, - OwnerReferences: []metav1.OwnerReference{{ - APIVersion: st.APIVersion, - Kind: st.Kind, - Name: st.GetObjectMeta().GetName(), - UID: st.GetObjectMeta().GetUID(), - }}, - Labels: map[string]string{ - scopeTemplateUIDKey: string(st.GetUID()), - scopeTemplateHashKey: util.HashObject(st.Spec), - clusterRoleGenerateKey: cr.GenerateName, - }, - }, - Rules: cr.Rules, - } + clusterRole := r.getClusterRole(&cr, st) - crbList := &rbacv1.ClusterRoleList{} - if err := r.Client.List(ctx, crbList, client.MatchingLabels{ + crList := &rbacv1.ClusterRoleList{} + if err := r.Client.List(ctx, crList, client.MatchingLabels{ scopeTemplateUIDKey: string(st.GetUID()), clusterRoleGenerateKey: cr.GenerateName, }); err != nil { @@ -182,34 +193,39 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op } // TODO: here to compare the clusterRoles against the expected values. - if len(crbList.Items) > 1 { + if len(crList.Items) > 1 { return fmt.Errorf("more than one ClusterRole found %s", cr.GenerateName) } // GenerateName is immutable, so create the object if it has changed - if len(crbList.Items) == 0 { + if len(crList.Items) == 0 { if err := r.Client.Create(ctx, clusterRole); err != nil { return err } continue } - existingCRB := &crbList.Items[0] + existingCR := &crList.Items[0] - if util.IsOwnedByLabel(existingCRB.DeepCopy(), st) && - reflect.DeepEqual(existingCRB.Rules, clusterRole.Rules) && - reflect.DeepEqual(existingCRB.Labels, clusterRole.Labels) { - log.Log.Info("existing ClusterRoleBinding does not need to be updated") + if util.IsOwnedByLabel(existingCR.DeepCopy(), st) && + reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) && + reflect.DeepEqual(existingCR.Labels, clusterRole.Labels) { + log.Log.Info("existing ClusterRole does not need to be updated") return nil } - existingCRB.Labels = clusterRole.Labels - existingCRB.OwnerReferences = clusterRole.OwnerReferences - existingCRB.Rules = clusterRole.Rules + + existingCR.Labels = clusterRole.Labels + existingCR.OwnerReferences = clusterRole.OwnerReferences + existingCR.Rules = clusterRole.Rules + + u, err := r.patchConfigForClusterRole(existingCR, clusterRole) + if err != nil { + return err + } // server-side apply patch - existingCRB.ManagedFields = nil if err := r.Client.Patch(ctx, - existingCRB, + u, client.Apply, client.FieldOwner(stCtrlFieldOwner), client.ForceOwnership); err != nil { @@ -219,6 +235,32 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } +func (r *ScopeTemplateReconciler) patchConfigForClusterRole(oldCr *rbacv1.ClusterRole, cr *rbacv1.ClusterRole) (*unstructured.Unstructured, error) { + crAc := rbacv1ac.ClusterRole(oldCr.Name).WithLabels(cr.Labels) + rulesAc := []rbacv1ac.PolicyRuleApplyConfiguration{} + orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} + + for _, rule := range cr.Rules { + ruleAc := *rbacv1ac.PolicyRule().WithAPIGroups(rule.APIGroups...).WithNonResourceURLs(rule.NonResourceURLs...).WithResourceNames(rule.ResourceNames...).WithResources(rule.Resources...).WithVerbs(rule.Verbs...) + rulesAc = append(rulesAc, ruleAc) + } + + for _, own := range cr.OwnerReferences { + ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) + orAcs = append(orAcs, ownAc) + } + + crAc.Rules = rulesAc + crAc.OwnerReferences = orAcs + + uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(crAc) + if err != nil { + return nil, err + } + + return &unstructured.Unstructured{Object: uMap}, nil +} + func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOptions ...client.ListOption) error { clusterRoles := &rbacv1.ClusterRoleList{} if err := r.Client.List(ctx, clusterRoles, listOptions...); err != nil { @@ -234,3 +276,20 @@ func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOp } return nil } + +func (r *ScopeTemplateReconciler) getClusterRole(crt *operatorsv1.ClusterRoleTemplate, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRole { + cr := &rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: crt.GenerateName, + Labels: map[string]string{ + scopeTemplateUIDKey: string(st.GetUID()), + scopeTemplateHashKey: util.HashObject(st.Spec), + clusterRoleGenerateKey: crt.GenerateName, + }, + }, + Rules: crt.Rules, + } + + ctrl.SetControllerReference(st, cr, r.Scheme) + return cr +} From 1063a29e19635244d1c9c0bce1382c517b70a5eb Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 16 Sep 2022 16:26:40 -0400 Subject: [PATCH 04/21] remove unecessary lines Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index 6d224d3..d163c13 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -214,10 +214,6 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } - existingCR.Labels = clusterRole.Labels - existingCR.OwnerReferences = clusterRole.OwnerReferences - existingCR.Rules = clusterRole.Rules - u, err := r.patchConfigForClusterRole(existingCR, clusterRole) if err != nil { return err From 9832f871da99e4bbc39f40fa8d556661222445e1 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 16 Sep 2022 16:32:07 -0400 Subject: [PATCH 05/21] ignore not found error Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index d163c13..110d501 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -77,7 +77,7 @@ func (r *ScopeTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Reques // get the scope template existingSt := &operatorsv1.ScopeTemplate{} if err := r.Client.Get(ctx, req.NamespacedName, existingSt); err != nil { - return ctrl.Result{}, err + return ctrl.Result{}, client.IgnoreNotFound(err) } // Perform reconciliation From e99a979253cb50b7abf599e7d6764a84fdcf9f01 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 20 Sep 2022 11:33:08 -0400 Subject: [PATCH 06/21] updates based on PR review Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index d4fe6ef..bd37f3d 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -147,8 +147,11 @@ func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1 return ctrl.Result{}, nil } +// ensureBindings will ensure that the proper bindings are created for a given ScopeInstance and ScopeTemplate. +// If the ScopeInstance.Spec.Namespaces is empty it will create a ClusterRoleBinding. +// If the ScopeInstance.Spec.Namespaces is not empty it will create a RoleBinding in each provided namespace. +// A separate (Cluster)RoleBinding will be created for each ClusterRole specified in the ScopeTemplate func (r *ScopeInstanceReconciler) ensureBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - // it will create clusterrole as shown below if no namespace is provided for _, cr := range st.Spec.ClusterRoles { if len(in.Spec.Namespaces) == 0 { err := r.createOrUpdateClusterRoleBinding(ctx, &cr, in, st) @@ -359,7 +362,13 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption } // deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. +// Being out of date means that the hash of the ScopeInstance.Spec is different OR the hash of the ScopeTemplate.Spec is different func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { + // TODO: It may be worth looking at combining the hash of the ScopeInstance.Spec and ScopeTemplate.Spec into a single hash to + // make it easier to facilitate the OR delete operation since the single hash would different if the ScopeInstance.Spec or + // ScopeTemplate.Spec is different than what we are expecting. + + // Delete bindings where the hash of the ScopeInstance.Spec is different siHashReq, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) if err != nil { return err @@ -378,6 +387,7 @@ func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *ope return err } + // Delete bindings where the hash of the ScopeTemplate.Spec is different stHashReq, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) if err != nil { return err From f80fcf25a2b332882d4f9f4d75a37ac1f40b6d4f Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 20 Sep 2022 13:32:07 -0400 Subject: [PATCH 07/21] update patch config to just return unstructured.Unstructured instead of dealing with applyconfigurations Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 89 ++++++++----------------- controllers/scopetemplate_controller.go | 42 ++++-------- 2 files changed, 40 insertions(+), 91 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index bd37f3d..63a1f4d 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -34,8 +34,6 @@ import ( "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" apimacherrors "k8s.io/apimachinery/pkg/util/errors" - metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" - rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -202,10 +200,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C return nil } - u, err := r.patchConfigForClusterRoleBinding(existingCRB, crb) - if err != nil { - return err - } + u := r.patchConfigForClusterRoleBinding(existingCRB, crb) // server-side apply patch if err := r.patchBinding(ctx, u); err != nil { @@ -215,33 +210,19 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C return nil } -func (r *ScopeInstanceReconciler) patchConfigForClusterRoleBinding(oldCrb *rbacv1.ClusterRoleBinding, crb *rbacv1.ClusterRoleBinding) (*unstructured.Unstructured, error) { - crbAc := rbacv1ac.ClusterRoleBinding(oldCrb.Name).WithLabels(crb.Labels) - subjAcs := []rbacv1ac.SubjectApplyConfiguration{} - orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} - for _, sub := range crb.Subjects { - subjAc := *rbacv1ac.Subject().WithAPIGroup(sub.APIGroup).WithKind(sub.Kind).WithName(sub.Name) - if sub.Namespace != "" { - subjAc.Namespace = &sub.Namespace - } - - subjAcs = append(subjAcs, subjAc) - } - for _, own := range crb.OwnerReferences { - ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) - orAcs = append(orAcs, ownAc) - } - - crbAc.OwnerReferences = orAcs - crbAc.Subjects = subjAcs - crbAc.RoleRef = rbacv1ac.RoleRef().WithAPIGroup(crb.RoleRef.APIGroup).WithKind(crb.RoleRef.Kind).WithName(crb.RoleRef.Name) - - uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(crbAc) - if err != nil { - return nil, err +func (r *ScopeInstanceReconciler) patchConfigForClusterRoleBinding(oldCrb *rbacv1.ClusterRoleBinding, crb *rbacv1.ClusterRoleBinding) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": rbacv1.SchemeGroupVersion.String(), + "kind": "ClusterRoleBinding", + "metadata": map[string]interface{}{ + "name": oldCrb.Name, + "ownerReferences": crb.OwnerReferences, + "labels": crb.Labels, + }, + "subjects": crb.Subjects, + }, } - - return &unstructured.Unstructured{Object: uMap}, nil } func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) error { @@ -280,10 +261,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, return nil } - u, err := r.patchConfigForRoleBinding(existingRB, rb) - if err != nil { - return err - } + u := r.patchConfigForRoleBinding(existingRB, rb) // server-side apply patch if err := r.patchBinding(ctx, u); err != nil { @@ -293,33 +271,20 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, return nil } -func (r *ScopeInstanceReconciler) patchConfigForRoleBinding(oldRb *rbacv1.RoleBinding, rb *rbacv1.RoleBinding) (*unstructured.Unstructured, error) { - rbAc := rbacv1ac.ClusterRoleBinding(oldRb.Name).WithLabels(rb.Labels) - subjAcs := []rbacv1ac.SubjectApplyConfiguration{} - orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} - for _, sub := range rb.Subjects { - subjAc := *rbacv1ac.Subject().WithAPIGroup(sub.APIGroup).WithKind(sub.Kind).WithName(sub.Name) - if sub.Namespace != "" { - subjAc.Namespace = &sub.Namespace - } - - subjAcs = append(subjAcs, subjAc) - } - for _, own := range rb.OwnerReferences { - ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) - orAcs = append(orAcs, ownAc) - } - - rbAc.OwnerReferences = orAcs - rbAc.Subjects = subjAcs - rbAc.RoleRef = rbacv1ac.RoleRef().WithAPIGroup(rb.RoleRef.APIGroup).WithKind(rb.RoleRef.Kind).WithName(rb.RoleRef.Name) - - uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rbAc) - if err != nil { - return nil, err +func (r *ScopeInstanceReconciler) patchConfigForRoleBinding(oldRb *rbacv1.RoleBinding, rb *rbacv1.RoleBinding) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": rbacv1.SchemeGroupVersion.String(), + "kind": "RoleBinding", + "metadata": map[string]interface{}{ + "name": oldRb.Name, + "namespace": oldRb.Namespace, + "ownerReferences": rb.OwnerReferences, + "labels": rb.Labels, + }, + "subjects": rb.Subjects, + }, } - - return &unstructured.Unstructured{Object: uMap}, nil } func (r *ScopeInstanceReconciler) patchBinding(ctx context.Context, binding client.Object) error { diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index 110d501..edaee65 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -34,8 +34,6 @@ import ( "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" apimacherrors "k8s.io/apimachinery/pkg/util/errors" - metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" - rbacv1ac "k8s.io/client-go/applyconfigurations/rbac/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -214,10 +212,7 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } - u, err := r.patchConfigForClusterRole(existingCR, clusterRole) - if err != nil { - return err - } + u := r.patchConfigForClusterRole(existingCR, clusterRole) // server-side apply patch if err := r.Client.Patch(ctx, @@ -231,30 +226,19 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } -func (r *ScopeTemplateReconciler) patchConfigForClusterRole(oldCr *rbacv1.ClusterRole, cr *rbacv1.ClusterRole) (*unstructured.Unstructured, error) { - crAc := rbacv1ac.ClusterRole(oldCr.Name).WithLabels(cr.Labels) - rulesAc := []rbacv1ac.PolicyRuleApplyConfiguration{} - orAcs := []metav1ac.OwnerReferenceApplyConfiguration{} - - for _, rule := range cr.Rules { - ruleAc := *rbacv1ac.PolicyRule().WithAPIGroups(rule.APIGroups...).WithNonResourceURLs(rule.NonResourceURLs...).WithResourceNames(rule.ResourceNames...).WithResources(rule.Resources...).WithVerbs(rule.Verbs...) - rulesAc = append(rulesAc, ruleAc) - } - - for _, own := range cr.OwnerReferences { - ownAc := *metav1ac.OwnerReference().WithAPIVersion(own.APIVersion).WithKind(own.Kind).WithName(own.Name).WithUID(own.UID) - orAcs = append(orAcs, ownAc) - } - - crAc.Rules = rulesAc - crAc.OwnerReferences = orAcs - - uMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(crAc) - if err != nil { - return nil, err +func (r *ScopeTemplateReconciler) patchConfigForClusterRole(oldCr *rbacv1.ClusterRole, cr *rbacv1.ClusterRole) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": rbacv1.SchemeGroupVersion.String(), + "kind": "ClusterRole", + "metadata": map[string]interface{}{ + "name": oldCr.Name, + "ownerReferences": cr.OwnerReferences, + "labels": cr.Labels, + }, + "rules": cr.Rules, + }, } - - return &unstructured.Unstructured{Object: uMap}, nil } func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOptions ...client.ListOption) error { From 19ff799f43169ddf8dec4cbe1032ffb378ac5daa Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 15:59:01 -0400 Subject: [PATCH 08/21] shared hash Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 38 ++++--------------------- controllers/scopetemplate_controller.go | 6 ++++ 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index a0e0f92..753e80f 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -51,11 +51,9 @@ type ScopeInstanceReconciler struct { const ( // UID keys are used to track "owners" of bindings we create. scopeInstanceUIDKey = "operators.coreos.io/scopeInstanceUID" - scopeTemplateUIDKey = "operators.coreos.io/scopeTemplateUID" // Hash keys are used to track "abandoned" bindings we created. - scopeInstanceHashKey = "operators.coreos.io/scopeInstanceHash" - scopeTemplateHashKey = "operators.coreos.io/scopeTemplateHash" + referenceHashKey = "operators.coreos.io/scopeInstanceAndTemplateHash" // generateNames are used to track each binding we create for a single scopeTemplate clusterRoleBindingGenerateKey = "operators.coreos.io/generateName" @@ -175,7 +173,6 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C crbList := &rbacv1.ClusterRoleBindingList{} if err := r.Client.List(ctx, crbList, client.MatchingLabels{ scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), clusterRoleBindingGenerateKey: cr.GenerateName, }); err != nil { return err @@ -233,7 +230,6 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, Namespace: namespace, }, client.MatchingLabels{ scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), clusterRoleBindingGenerateKey: cr.GenerateName, }); err != nil { return err @@ -335,7 +331,8 @@ func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *ope // ScopeTemplate.Spec is different than what we are expecting. // Delete bindings where the hash of the ScopeInstance.Spec is different - siHashReq, err := labels.NewRequirement(scopeInstanceHashKey, selection.NotEquals, []string{util.HashObject(in.Spec)}) + combinedHash := util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)) + hashReq, err := labels.NewRequirement(referenceHashKey, selection.NotEquals, []string{combinedHash}) if err != nil { return err } @@ -346,26 +343,7 @@ func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *ope } listOptions := &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*siHashReq, *siUIDReq), - } - - if err := r.deleteBindings(ctx, listOptions); err != nil { - return err - } - - // Delete bindings where the hash of the ScopeTemplate.Spec is different - stHashReq, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) - if err != nil { - return err - } - - stUIDReq, err := labels.NewRequirement(scopeTemplateUIDKey, selection.Equals, []string{string(st.GetUID())}) - if err != nil { - return err - } - - listOptions = &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*siUIDReq, *stUIDReq, *stHashReq), + LabelSelector: labels.NewSelector().Add(*hashReq, *siUIDReq), } if err := r.deleteBindings(ctx, listOptions); err != nil { @@ -418,9 +396,7 @@ func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterR GenerateName: cr.GenerateName + "-", Labels: map[string]string{ scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - scopeInstanceHashKey: util.HashObject(in.Spec), - scopeTemplateHashKey: util.HashObject(st.Spec), + referenceHashKey: util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)), clusterRoleBindingGenerateKey: cr.GenerateName, }, }, @@ -444,9 +420,7 @@ func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemp Namespace: namespace, Labels: map[string]string{ scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), - scopeInstanceHashKey: util.HashObject(in.Spec), - scopeTemplateHashKey: util.HashObject(st.Spec), + referenceHashKey: util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)), clusterRoleBindingGenerateKey: cr.GenerateName, }, }, diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index e2255fb..a457041 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -49,6 +49,12 @@ type ScopeTemplateReconciler struct { } const ( + // UID keys are used to track "owners" of bindings we create. + scopeTemplateUIDKey = "operators.coreos.io/scopeTemplateUID" + + // Hash keys are used to track "abandoned" bindings we created. + scopeTemplateHashKey = "operators.coreos.io/scopeTemplateHash" + // generateNames are used to track each binding we create for a single scopeTemplate clusterRoleGenerateKey = "operators.coreos.io/generateName" stCtrlFieldOwner = "scopetemplate-controller" From 6eeb5d8796db2604bcbd4b0b3abc764139a8bd00 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 16:01:30 -0400 Subject: [PATCH 09/21] remove unnecessary comments Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 753e80f..7511406 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -326,11 +326,6 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption // deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. // Being out of date means that the hash of the ScopeInstance.Spec is different OR the hash of the ScopeTemplate.Spec is different func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - // TODO: It may be worth looking at combining the hash of the ScopeInstance.Spec and ScopeTemplate.Spec into a single hash to - // make it easier to facilitate the OR delete operation since the single hash would different if the ScopeInstance.Spec or - // ScopeTemplate.Spec is different than what we are expecting. - - // Delete bindings where the hash of the ScopeInstance.Spec is different combinedHash := util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)) hashReq, err := labels.NewRequirement(referenceHashKey, selection.NotEquals, []string{combinedHash}) if err != nil { From 8cbe23ccd48903a1f9f23f4c806412dc6aecd8b3 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 16:06:03 -0400 Subject: [PATCH 10/21] fix lint errors Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 11 ++++++++--- controllers/scopetemplate_controller.go | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 7511406..739f409 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -403,7 +403,10 @@ func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterR }, } - ctrl.SetControllerReference(in, crb, r.Scheme) + err := ctrl.SetControllerReference(in, crb, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } return crb } @@ -427,7 +430,9 @@ func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemp }, } - ctrl.SetControllerReference(in, rb, r.Scheme) + err := ctrl.SetControllerReference(in, rb, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } return rb - } diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index a457041..446eb5e 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -277,6 +277,9 @@ func (r *ScopeTemplateReconciler) getClusterRole(crt *operatorsv1.ClusterRoleTem Rules: crt.Rules, } - ctrl.SetControllerReference(st, cr, r.Scheme) + err := ctrl.SetControllerReference(st, cr, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } return cr } From ac6787c3179780ea574aa50ed1318dfc66523a66 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 16:50:05 -0400 Subject: [PATCH 11/21] fix unit tests Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller_test.go | 36 +++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/controllers/scopetemplate_controller_test.go b/controllers/scopetemplate_controller_test.go index 1a65ff1..7ffa796 100644 --- a/controllers/scopetemplate_controller_test.go +++ b/controllers/scopetemplate_controller_test.go @@ -25,6 +25,7 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" operatorsv1 "awgreene/scope-operator/api/v1alpha1" @@ -142,10 +143,12 @@ var _ = Describe("ScopeTemplate", func() { //TODO: Non-blocking: Let's create functions to check verify the clusterRole and clusterRoleBindings in a followup PR. Expect(len(existingRole.OwnerReferences)).Should(Equal(1)) Expect(existingRole.OwnerReferences).Should(ContainElement(metav1.OwnerReference{ - APIVersion: "operators.io.operator-framework/v1alpha1", - Kind: "ScopeTemplate", - Name: scopeTemplate.GetName(), - UID: scopeTemplate.GetUID(), + APIVersion: "operators.io.operator-framework/v1alpha1", + Kind: "ScopeTemplate", + Name: scopeTemplate.GetName(), + UID: scopeTemplate.GetUID(), + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), })) Expect(existingRole.Rules).Should(Equal([]rbacv1.PolicyRule{ { @@ -158,7 +161,6 @@ var _ = Describe("ScopeTemplate", func() { It("should create the expected RoleBinding within the test namespace", func() { labels := map[string]string{scopeInstanceUIDKey: string(scopeInstance.GetUID()), - scopeTemplateUIDKey: string(scopeTemplate.GetUID()), clusterRoleBindingGenerateKey: "test"} roleBindingList := listRoleBinding(namespace.GetName(), 1, labels) @@ -196,7 +198,6 @@ var _ = Describe("ScopeTemplate", func() { }, timeout, interval).Should(BeNil()) labels := map[string]string{scopeInstanceUIDKey: string(scopeInstance.GetUID()), - scopeTemplateUIDKey: string(scopeTemplate.GetUID()), clusterRoleBindingGenerateKey: "test"} roleBindingList := listRoleBinding(namespace2.GetName(), 1, labels) @@ -226,7 +227,6 @@ var _ = Describe("ScopeTemplate", func() { }, timeout, interval).Should(BeNil()) labels := map[string]string{scopeInstanceUIDKey: string(scopeInstance.GetUID()), - scopeTemplateUIDKey: string(scopeTemplate.GetUID()), clusterRoleBindingGenerateKey: "test"} roleBindingList := listRoleBinding(namespace2.GetName(), 1, labels) @@ -258,7 +258,6 @@ var _ = Describe("ScopeTemplate", func() { err := k8sClient.List(ctx, clusterRoleBindingList, client.MatchingLabels{ scopeInstanceUIDKey: string(scopeInstance.GetUID()), - scopeTemplateUIDKey: string(scopeTemplate.GetUID()), clusterRoleBindingGenerateKey: "test", }) if err != nil { @@ -276,10 +275,12 @@ var _ = Describe("ScopeTemplate", func() { Expect(len(existingCRB.OwnerReferences)).To(Equal(1)) Expect(existingCRB.OwnerReferences).Should(ContainElement(metav1.OwnerReference{ - APIVersion: "operators.io.operator-framework/v1alpha1", - Kind: "ScopeInstance", - Name: scopeInstance.GetObjectMeta().GetName(), - UID: scopeInstance.GetObjectMeta().GetUID(), + APIVersion: "operators.io.operator-framework/v1alpha1", + Kind: "ScopeInstance", + Name: scopeInstance.GetObjectMeta().GetName(), + UID: scopeInstance.GetObjectMeta().GetUID(), + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), })) Expect(len(existingCRB.Subjects)).To(Equal(1)) @@ -295,7 +296,6 @@ var _ = Describe("ScopeTemplate", func() { })) labels := map[string]string{scopeInstanceUIDKey: string(scopeInstance.GetUID()), - scopeTemplateUIDKey: string(scopeTemplate.GetUID()), clusterRoleBindingGenerateKey: "test"} roleBindingList := listRoleBinding(namespace.GetName(), 0, labels) @@ -315,10 +315,12 @@ func verifyRoleBindings(existingRB *rbacv1.RoleBinding, si *operatorsv1.ScopeIns // verify cluster role bindings with ownerference, subjects, and role reference. Expect(len(existingRB.OwnerReferences)).To(Equal(1)) Expect(existingRB.OwnerReferences).Should(ContainElement(metav1.OwnerReference{ - APIVersion: "operators.io.operator-framework/v1alpha1", - Kind: "ScopeInstance", - Name: si.GetObjectMeta().GetName(), - UID: si.GetObjectMeta().GetUID(), + APIVersion: "operators.io.operator-framework/v1alpha1", + Kind: "ScopeInstance", + Name: si.GetObjectMeta().GetName(), + UID: si.GetObjectMeta().GetUID(), + Controller: pointer.Bool(true), + BlockOwnerDeletion: pointer.Bool(true), })) Expect(len(existingRB.Subjects)).To(Equal(1)) From 18514fd6fe241af8b05ca67b1414c4baa67b2b38 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 16:57:16 -0400 Subject: [PATCH 12/21] update todo in mapToScopeTemplate Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index 446eb5e..eb3a0d5 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -164,10 +164,8 @@ func (r *ScopeTemplateReconciler) mapToScopeTemplate(obj client.Object) (request return } - ctx := context.TODO() - //(todo): Check if obj can be converted into a scope instance. - scopeInstance := &operatorsv1.ScopeInstance{} - if err := r.Client.Get(ctx, types.NamespacedName{Name: obj.GetName()}, scopeInstance); err != nil { + scopeInstance, ok := obj.(*operatorsv1.ScopeInstance) + if !ok { return nil } From a6a04ce8dc82be9eefdb5f44b2e08d7653034778 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 21 Sep 2022 16:57:47 -0400 Subject: [PATCH 13/21] make verify Signed-off-by: Bryce Palmer --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index cd3c5cb..01b59a6 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( k8s.io/api v0.24.4 k8s.io/apimachinery v0.24.4 k8s.io/client-go v0.24.4 + k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 sigs.k8s.io/controller-runtime v0.12.1 ) @@ -77,7 +78,6 @@ require ( k8s.io/component-base v0.24.4 // indirect k8s.io/klog/v2 v2.60.1 // indirect k8s.io/kube-openapi v0.0.0-20220328201542-3ee0da9b0b42 // indirect - k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 // indirect sigs.k8s.io/json v0.0.0-20211208200746-9f7c6b3444d2 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.1 // indirect sigs.k8s.io/yaml v1.3.0 // indirect From 7c180d6392c3ecb5d05bcb24ad8c25e311404670 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Mon, 26 Sep 2022 10:23:13 -0400 Subject: [PATCH 14/21] update hashing method in scopeinstance controller Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 739f409..ba8bd9c 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -326,7 +326,7 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption // deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. // Being out of date means that the hash of the ScopeInstance.Spec is different OR the hash of the ScopeTemplate.Spec is different func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - combinedHash := util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)) + combinedHash := hashScopeInstanceAndTemplate(in, st) hashReq, err := labels.NewRequirement(referenceHashKey, selection.NotEquals, []string{combinedHash}) if err != nil { return err @@ -391,7 +391,7 @@ func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterR GenerateName: cr.GenerateName + "-", Labels: map[string]string{ scopeInstanceUIDKey: string(in.GetUID()), - referenceHashKey: util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)), + referenceHashKey: hashScopeInstanceAndTemplate(in, st), clusterRoleBindingGenerateKey: cr.GenerateName, }, }, @@ -418,7 +418,7 @@ func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemp Namespace: namespace, Labels: map[string]string{ scopeInstanceUIDKey: string(in.GetUID()), - referenceHashKey: util.HashObject(util.HashObject(in.Spec) + util.HashObject(st.Spec)), + referenceHashKey: hashScopeInstanceAndTemplate(in, st), clusterRoleBindingGenerateKey: cr.GenerateName, }, }, @@ -436,3 +436,22 @@ func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemp } return rb } + +// referenchHash is used to store a ScopeInstance.Spec +// and ScopeTemplate.Spec. This object is used for getting +// the combined hash of both specs. +type referenceHash struct { + ScopeInstanceSpec *operatorsv1.ScopeInstanceSpec + ScopeTemplateSpec *operatorsv1.ScopeTemplateSpec +} + +// hashScopeInstanceAndTemplate will take in a ScopeInstance and ScopeTemplate and return +// a combined hash of the ScopeInstance.Spec and ScopeTemplate.Spec fields +func hashScopeInstanceAndTemplate(si *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) string { + hashObj := &referenceHash{ + ScopeInstanceSpec: &si.Spec, + ScopeTemplateSpec: &st.Spec, + } + + return util.HashObject(hashObj) +} From 0d806dd453fda2c2a66e0739b7c3cf83500adea8 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Mon, 26 Sep 2022 15:08:51 -0400 Subject: [PATCH 15/21] first round of PR review fixes Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 21 ++++++++++----------- controllers/scopetemplate_controller.go | 4 ++-- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index ba8bd9c..a32225a 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -49,10 +49,10 @@ type ScopeInstanceReconciler struct { } const ( - // UID keys are used to track "owners" of bindings we create. + // scopeInstanceUIDKey is used to track "owners" of bindings we create. scopeInstanceUIDKey = "operators.coreos.io/scopeInstanceUID" - // Hash keys are used to track "abandoned" bindings we created. + // referenceHashKey is used to track "abandoned" bindings we created. referenceHashKey = "operators.coreos.io/scopeInstanceAndTemplateHash" // generateNames are used to track each binding we create for a single scopeTemplate @@ -77,14 +77,13 @@ const ( func (r *ScopeInstanceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { _ = log.FromContext(ctx) - log.Log.Info("Reconciling ScopeInstance", "namespaceName", req.NamespacedName) + log.Log.V(2).Info("Reconciling ScopeInstance", "namespaceName", req.NamespacedName) existingIn := &operatorsv1.ScopeInstance{} if err := r.Client.Get(ctx, req.NamespacedName, existingIn); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - // Perform reconciliation reconciledIn := existingIn.DeepCopy() res, reconcileErr := r.reconcile(ctx, reconciledIn) @@ -122,7 +121,7 @@ func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1 } if err := r.deleteBindings(ctx, listOption); err != nil { - log.Log.Error(err, "in deleting (Cluster)RoleBindings") + log.Log.V(2).Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } @@ -131,13 +130,13 @@ func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1 // create required roleBindings and clusterRoleBindings. if err := r.ensureBindings(ctx, in, st); err != nil { - log.Log.Error(err, "in creating RoleBindings") + log.Log.V(2).Error(err, "in creating RoleBindings") return ctrl.Result{}, err } // delete out of date (Cluster)RoleBindings if err := r.deleteOldBindings(ctx, in, st); err != nil { - log.Log.Error(err, "in deleting (Cluster)RoleBindings") + log.Log.V(2).Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } @@ -194,7 +193,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C if util.IsOwnedByLabel(existingCRB.DeepCopy(), in) && reflect.DeepEqual(existingCRB.Subjects, crb.Subjects) && reflect.DeepEqual(existingCRB.Labels, crb.Labels) { - log.Log.Info("existing ClusterRoleBinding does not need to be updated") + log.Log.V(2).Info("existing ClusterRoleBinding does not need to be updated") return nil } @@ -247,14 +246,14 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, return nil } - log.Log.Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) + log.Log.V(2).Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) existingRB := &rbList.Items[0] if util.IsOwnedByLabel(existingRB.DeepCopy(), in) && reflect.DeepEqual(existingRB.Subjects, rb.Subjects) && reflect.DeepEqual(existingRB.Labels, rb.Labels) { - log.Log.Info("existing RoleBinding does not need to be updated") + log.Log.V(2).Info("existing RoleBinding does not need to be updated") return nil } @@ -437,7 +436,7 @@ func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemp return rb } -// referenchHash is used to store a ScopeInstance.Spec +// referenceHash is used to store a ScopeInstance.Spec // and ScopeTemplate.Spec. This object is used for getting // the combined hash of both specs. type referenceHash struct { diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index eb3a0d5..a4e359b 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -126,13 +126,13 @@ func (r *ScopeTemplateReconciler) reconcile(ctx context.Context, st *operatorsv1 } } - // Add requirement to delete old hashes + // Add requirement to delete old (Cluster)Roles stHashReq, err := labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) if err != nil { return ctrl.Result{}, err } - // Only look for old clusterroles that map to this ScopeTemplate UID + // Only look for old (Cluster)Roles that map to this ScopeTemplate UID stUIDReq, err := labels.NewRequirement(scopeTemplateUIDKey, selection.Equals, []string{string(st.GetUID())}) if err != nil { return ctrl.Result{}, err From 77b7c6f630462f260b85e7e8d3c753944298c53b Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 27 Sep 2022 10:49:20 -0400 Subject: [PATCH 16/21] second round of PR review updates Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 55 ++++++++++++++----------- controllers/scopetemplate_controller.go | 19 ++++----- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index a32225a..69090ac 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -143,10 +143,12 @@ func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1 return ctrl.Result{}, nil } -// ensureBindings will ensure that the proper bindings are created for a given ScopeInstance and ScopeTemplate. -// If the ScopeInstance.Spec.Namespaces is empty it will create a ClusterRoleBinding. -// If the ScopeInstance.Spec.Namespaces is not empty it will create a RoleBinding in each provided namespace. -// A separate (Cluster)RoleBinding will be created for each ClusterRole specified in the ScopeTemplate +// ensureBindings will ensure that the proper bindings are created for a +// given ScopeInstance and ScopeTemplate. If the ScopeInstance.Spec.Namespaces +// is empty it will create a ClusterRoleBinding. If the +// ScopeInstance.Spec.Namespaces is not empty it will create a RoleBinding +// in each provided namespace. A separate (Cluster)RoleBinding will be created +// for each ClusterRole specified in the ScopeTemplate func (r *ScopeInstanceReconciler) ensureBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { for _, cr := range st.Spec.ClusterRoles { if len(in.Spec.Namespaces) == 0 { @@ -168,7 +170,7 @@ func (r *ScopeInstanceReconciler) ensureBindings(ctx context.Context, in *operat } func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { - crb := r.getClusterRoleBinding(cr, in, st) + crb := r.clusterRoleBindingManifest(cr, in, st) crbList := &rbacv1.ClusterRoleBindingList{} if err := r.Client.List(ctx, crbList, client.MatchingLabels{ scopeInstanceUIDKey: string(in.GetUID()), @@ -181,7 +183,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C return fmt.Errorf("more than one ClusterRoleBinding found for ClusterRole %s", cr.GenerateName) } - // GenerateName is immutable, so create the object if it has changed + // Create the ClusterRoleBinding if one doesn't already exist if len(crbList.Items) == 0 { if err := r.Client.Create(ctx, crb); err != nil { return err @@ -193,21 +195,21 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C if util.IsOwnedByLabel(existingCRB.DeepCopy(), in) && reflect.DeepEqual(existingCRB.Subjects, crb.Subjects) && reflect.DeepEqual(existingCRB.Labels, crb.Labels) { - log.Log.V(2).Info("existing ClusterRoleBinding does not need to be updated") + log.Log.V(2).Info("existing ClusterRoleBinding does not need to be updated", "UID", existingCRB.GetUID()) return nil } - u := r.patchConfigForClusterRoleBinding(existingCRB, crb) + patchObj := r.clusterRoleBindingPatchObj(existingCRB, crb) // server-side apply patch - if err := r.patchBinding(ctx, u); err != nil { + if err := r.patchBinding(ctx, patchObj); err != nil { return err } return nil } -func (r *ScopeInstanceReconciler) patchConfigForClusterRoleBinding(oldCrb *rbacv1.ClusterRoleBinding, crb *rbacv1.ClusterRoleBinding) *unstructured.Unstructured { +func (r *ScopeInstanceReconciler) clusterRoleBindingPatchObj(oldCrb *rbacv1.ClusterRoleBinding, crb *rbacv1.ClusterRoleBinding) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": rbacv1.SchemeGroupVersion.String(), @@ -223,7 +225,7 @@ func (r *ScopeInstanceReconciler) patchConfigForClusterRoleBinding(oldCrb *rbacv } func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) error { - rb := r.getRoleBinding(cr, in, st, namespace) + rb := r.roleBindingManifest(cr, in, st, namespace) rbList := &rbacv1.RoleBindingList{} if err := r.Client.List(ctx, rbList, &client.ListOptions{ Namespace: namespace, @@ -238,7 +240,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, return fmt.Errorf("more than one RoleBinding found for ClusterRole %s", cr.GenerateName) } - // GenerateName is immutable, so create the object if it has changed + // Create the RoleBinding if one doesn't already exist if len(rbList.Items) == 0 { if err := r.Client.Create(ctx, rb); err != nil { return err @@ -253,21 +255,21 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, if util.IsOwnedByLabel(existingRB.DeepCopy(), in) && reflect.DeepEqual(existingRB.Subjects, rb.Subjects) && reflect.DeepEqual(existingRB.Labels, rb.Labels) { - log.Log.V(2).Info("existing RoleBinding does not need to be updated") + log.Log.V(2).Info("existing RoleBinding does not need to be updated", "UID", existingRB.GetUID()) return nil } - u := r.patchConfigForRoleBinding(existingRB, rb) + patchObj := r.roleBindingPatchObj(existingRB, rb) // server-side apply patch - if err := r.patchBinding(ctx, u); err != nil { + if err := r.patchBinding(ctx, patchObj); err != nil { return err } return nil } -func (r *ScopeInstanceReconciler) patchConfigForRoleBinding(oldRb *rbacv1.RoleBinding, rb *rbacv1.RoleBinding) *unstructured.Unstructured { +func (r *ScopeInstanceReconciler) roleBindingPatchObj(oldRb *rbacv1.RoleBinding, rb *rbacv1.RoleBinding) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": rbacv1.SchemeGroupVersion.String(), @@ -322,8 +324,9 @@ func (r *ScopeInstanceReconciler) deleteBindings(ctx context.Context, listOption return nil } -// deleteOldBindings will delete any (Cluster)RoleBindings that are owned by the given ScopeInstance and are no longer up to date. -// Being out of date means that the hash of the ScopeInstance.Spec is different OR the hash of the ScopeTemplate.Spec is different +// deleteOldBindings will delete any (Cluster)RoleBindings that are owned by +// the given ScopeInstance and are no longer up to date.Being out of date +// means the combined hash of ScopeInstance.Spec and ScopeTemplate.Spec is different func (r *ScopeInstanceReconciler) deleteOldBindings(ctx context.Context, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { combinedHash := hashScopeInstanceAndTemplate(in, st) hashReq, err := labels.NewRequirement(referenceHashKey, selection.NotEquals, []string{combinedHash}) @@ -383,8 +386,9 @@ func (r *ScopeInstanceReconciler) mapToScopeInstance(obj client.Object) (request return } -// getClusterRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate, ScopeInstance, and ScopeTemplate -func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRoleBinding { +// clusterRoleBindingManifest will create a ClusterRoleBinding from a +// ClusterRoleTemplate, ScopeInstance, and ScopeTemplate +func (r *ScopeInstanceReconciler) clusterRoleBindingManifest(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRoleBinding { crb := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ GenerateName: cr.GenerateName + "-", @@ -409,8 +413,9 @@ func (r *ScopeInstanceReconciler) getClusterRoleBinding(cr *operatorsv1.ClusterR return crb } -// getRoleBindingForClusterRoleTemplate will create a ClusterRoleBinding from a ClusterRoleTemplate -func (r *ScopeInstanceReconciler) getRoleBinding(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) *rbacv1.RoleBinding { +// roleBindingManifest will create a RoleBinding from a +// ClusterRoleTemplate, ScopeInstance, ScopeTemplate, and namespace +func (r *ScopeInstanceReconciler) roleBindingManifest(cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) *rbacv1.RoleBinding { rb := &rbacv1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ GenerateName: cr.GenerateName + "-", @@ -444,8 +449,10 @@ type referenceHash struct { ScopeTemplateSpec *operatorsv1.ScopeTemplateSpec } -// hashScopeInstanceAndTemplate will take in a ScopeInstance and ScopeTemplate and return -// a combined hash of the ScopeInstance.Spec and ScopeTemplate.Spec fields +// hashScopeInstanceAndTemplate will take in a +// ScopeInstance and ScopeTemplate and return +// a combined hash of the ScopeInstance.Spec and +// ScopeTemplate.Spec fields func hashScopeInstanceAndTemplate(si *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) string { hashObj := &referenceHash{ ScopeInstanceSpec: &si.Spec, diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index a4e359b..2130127 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -49,10 +49,10 @@ type ScopeTemplateReconciler struct { } const ( - // UID keys are used to track "owners" of bindings we create. + // scopeTemplateUIDKey is used to track "owners" of (Cluster)Roles we create. scopeTemplateUIDKey = "operators.coreos.io/scopeTemplateUID" - // Hash keys are used to track "abandoned" bindings we created. + // scopeTemplateHashKey is used to track "abandoned" (Cluster)Roles we created. scopeTemplateHashKey = "operators.coreos.io/scopeTemplateHash" // generateNames are used to track each binding we create for a single scopeTemplate @@ -85,7 +85,6 @@ func (r *ScopeTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, client.IgnoreNotFound(err) } - // Perform reconciliation reconciledSt := existingSt.DeepCopy() res, reconcileErr := r.reconcile(ctx, reconciledSt) @@ -185,7 +184,7 @@ func (r *ScopeTemplateReconciler) mapToScopeTemplate(obj client.Object) (request func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *operatorsv1.ScopeTemplate) error { for _, cr := range st.Spec.ClusterRoles { - clusterRole := r.getClusterRole(&cr, st) + clusterRole := r.clusterRoleManifest(&cr, st) crList := &rbacv1.ClusterRoleList{} if err := r.Client.List(ctx, crList, client.MatchingLabels{ @@ -200,7 +199,7 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return fmt.Errorf("more than one ClusterRole found %s", cr.GenerateName) } - // GenerateName is immutable, so create the object if it has changed + // Create the ClusterRole if it does not exist if len(crList.Items) == 0 { if err := r.Client.Create(ctx, clusterRole); err != nil { return err @@ -213,15 +212,15 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op if util.IsOwnedByLabel(existingCR.DeepCopy(), st) && reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) && reflect.DeepEqual(existingCR.Labels, clusterRole.Labels) { - log.Log.Info("existing ClusterRole does not need to be updated") + log.Log.Info("existing ClusterRole does not need to be updated", "UID", existingCR.GetUID()) return nil } - u := r.patchConfigForClusterRole(existingCR, clusterRole) + patchObj := r.clusterRolePatchObj(existingCR, clusterRole) // server-side apply patch if err := r.Client.Patch(ctx, - u, + patchObj, client.Apply, client.FieldOwner(stCtrlFieldOwner), client.ForceOwnership); err != nil { @@ -231,7 +230,7 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } -func (r *ScopeTemplateReconciler) patchConfigForClusterRole(oldCr *rbacv1.ClusterRole, cr *rbacv1.ClusterRole) *unstructured.Unstructured { +func (r *ScopeTemplateReconciler) clusterRolePatchObj(oldCr *rbacv1.ClusterRole, cr *rbacv1.ClusterRole) *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": rbacv1.SchemeGroupVersion.String(), @@ -262,7 +261,7 @@ func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOp return nil } -func (r *ScopeTemplateReconciler) getClusterRole(crt *operatorsv1.ClusterRoleTemplate, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRole { +func (r *ScopeTemplateReconciler) clusterRoleManifest(crt *operatorsv1.ClusterRoleTemplate, st *operatorsv1.ScopeTemplate) *rbacv1.ClusterRole { cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ Name: crt.GenerateName, From 5d6a4f9de2ac93bc1bd977eec67a61e002fe7a75 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 27 Sep 2022 14:37:20 -0400 Subject: [PATCH 17/21] updated missed log line to be debug level Signed-off-by: Bryce Palmer --- controllers/scopetemplate_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index 2130127..39dc2dc 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -212,7 +212,7 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op if util.IsOwnedByLabel(existingCR.DeepCopy(), st) && reflect.DeepEqual(existingCR.Rules, clusterRole.Rules) && reflect.DeepEqual(existingCR.Labels, clusterRole.Labels) { - log.Log.Info("existing ClusterRole does not need to be updated", "UID", existingCR.GetUID()) + log.Log.V(2).Info("existing ClusterRole does not need to be updated", "UID", existingCR.GetUID()) return nil } From b8d6c2d762a0f59b4551a3aa1da45c3cd6e78c0e Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Tue, 27 Sep 2022 15:37:16 -0400 Subject: [PATCH 18/21] update log message Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 69090ac..71a164f 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -130,7 +130,7 @@ func (r *ScopeInstanceReconciler) reconcile(ctx context.Context, in *operatorsv1 // create required roleBindings and clusterRoleBindings. if err := r.ensureBindings(ctx, in, st); err != nil { - log.Log.V(2).Error(err, "in creating RoleBindings") + log.Log.V(2).Error(err, "in creating (Cluster)RoleBindings") return ctrl.Result{}, err } From 7c6c2cdc59a483931ed2d802db0e273731a36a1a Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 28 Sep 2022 17:40:56 -0400 Subject: [PATCH 19/21] Update controllers/scopeinstance_controller.go Co-authored-by: Alexander Greene --- controllers/scopeinstance_controller.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 71a164f..8e1860d 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -185,10 +185,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.C // Create the ClusterRoleBinding if one doesn't already exist if len(crbList.Items) == 0 { - if err := r.Client.Create(ctx, crb); err != nil { - return err - } - return nil + return r.Client.Create(ctx, crb) } existingCRB := &crbList.Items[0] From a7c6d41539179caa64d2702b6da04b0a95339418 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Wed, 28 Sep 2022 17:41:01 -0400 Subject: [PATCH 20/21] Update controllers/scopeinstance_controller.go Co-authored-by: Alexander Greene --- controllers/scopeinstance_controller.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 8e1860d..dd32897 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -239,9 +239,7 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, // Create the RoleBinding if one doesn't already exist if len(rbList.Items) == 0 { - if err := r.Client.Create(ctx, rb); err != nil { - return err - } + return r.Client.Create(ctx, rb) return nil } From bdbacf1d7bd759dc3ce69d9f7a9fda9fbb313b5b Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Mon, 3 Oct 2022 09:31:44 -0400 Subject: [PATCH 21/21] fix lint errors Signed-off-by: Bryce Palmer --- controllers/scopeinstance_controller.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index dd32897..1134ea4 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -240,7 +240,6 @@ func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, // Create the RoleBinding if one doesn't already exist if len(rbList.Items) == 0 { return r.Client.Create(ctx, rb) - return nil } log.Log.V(2).Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName())