diff --git a/controllers/scopeinstance_controller.go b/controllers/scopeinstance_controller.go index 50b04bc..1134ea4 100644 --- a/controllers/scopeinstance_controller.go +++ b/controllers/scopeinstance_controller.go @@ -25,12 +25,15 @@ 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" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -46,13 +49,11 @@ 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" - 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 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 clusterRoleBindingGenerateKey = "operators.coreos.io/generateName" @@ -76,13 +77,38 @@ 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) - 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) + } + + 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) { @@ -95,7 +121,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.V(2).Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } @@ -104,208 +130,161 @@ 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.V(2).Error(err, "in creating (Cluster)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.V(2).Error(err, "in deleting (Cluster)RoleBindings") return ctrl.Result{}, err } - listOptions := &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), - } + return ctrl.Result{}, nil +} - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { - log.Log.Error(err, "in deleting Role Bindings") - return ctrl.Result{}, err +// 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 { + 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 + } + } + } } - // TODO: Find out how to merge with the above delete - listOption = client.MatchingLabels{ - scopeInstanceUIDKey: string(in.GetUID()), - scopeTemplateUIDKey: string(st.GetUID()), + return nil +} + +func (r *ScopeInstanceReconciler) createOrUpdateClusterRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate) error { + crb := r.clusterRoleBindingManifest(cr, in, st) + crbList := &rbacv1.ClusterRoleBindingList{} + if err := r.Client.List(ctx, crbList, client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + clusterRoleBindingGenerateKey: cr.GenerateName, + }); err != nil { + return err } - requirement, err = labels.NewRequirement(scopeTemplateHashKey, selection.NotEquals, []string{util.HashObject(st.Spec)}) - if err != nil { - return ctrl.Result{}, err + if len(crbList.Items) > 1 { + return fmt.Errorf("more than one ClusterRoleBinding found for ClusterRole %s", cr.GenerateName) } - listOptions = &client.ListOptions{ - LabelSelector: labels.NewSelector().Add(*requirement), + // Create the ClusterRoleBinding if one doesn't already exist + if len(crbList.Items) == 0 { + return r.Client.Create(ctx, crb) } - if err := r.deleteBindings(ctx, listOption, listOptions); err != nil { - log.Log.Error(err, "in deleting Role Bindings") - return ctrl.Result{}, err + existingCRB := &crbList.Items[0] + 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", "UID", existingCRB.GetUID()) + return nil } - log.Log.Info("No ScopeInstance error") + patchObj := r.clusterRoleBindingPatchObj(existingCRB, crb) - return ctrl.Result{}, nil -} + // server-side apply patch + if err := r.patchBinding(ctx, patchObj); err != nil { + return err + } -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", - }, - } + return nil +} - 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 - } +func (r *ScopeInstanceReconciler) clusterRoleBindingPatchObj(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, + }, + } +} - if len(crbList.Items) > 1 { - return fmt.Errorf("more than one ClusterRoleBinding found for ClusterRole %s", cr.GenerateName) - } +func (r *ScopeInstanceReconciler) createOrUpdateRoleBinding(ctx context.Context, cr *operatorsv1.ClusterRoleTemplate, in *operatorsv1.ScopeInstance, st *operatorsv1.ScopeTemplate, namespace string) error { + rb := r.roleBindingManifest(cr, in, st, namespace) + rbList := &rbacv1.RoleBindingList{} + if err := r.Client.List(ctx, rbList, &client.ListOptions{ + Namespace: namespace, + }, client.MatchingLabels{ + scopeInstanceUIDKey: string(in.GetUID()), + clusterRoleBindingGenerateKey: cr.GenerateName, + }); err != nil { + return 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 - } - continue - } + if len(rbList.Items) > 1 { + return fmt.Errorf("more than one RoleBinding found for ClusterRole %s", cr.GenerateName) + } - existingCRB := &crbList.Items[0] + // Create the RoleBinding if one doesn't already exist + if len(rbList.Items) == 0 { + return r.Client.Create(ctx, rb) + } - 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 - } + log.Log.V(2).Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) - } - } 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", - }, - } + existingRB := &rbList.Items[0] - 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) - } + 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", "UID", existingRB.GetUID()) + return nil + } - // 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 - } + patchObj := r.roleBindingPatchObj(existingRB, rb) - log.Log.Info("Updating existing rb", "namespaced", rbList.Items[0].GetNamespace(), "name", rbList.Items[0].GetName()) + // server-side apply patch + if err := r.patchBinding(ctx, patchObj); err != nil { + return err + } - existingRB := &rbList.Items[0] + return nil +} - 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 - } - } - } +func (r *ScopeInstanceReconciler) roleBindingPatchObj(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 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 @@ -339,6 +318,32 @@ 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 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}) + if err != nil { + return err + } + + siUIDReq, err := labels.NewRequirement(scopeInstanceUIDKey, selection.Equals, []string{string(in.GetUID())}) + if err != nil { + return err + } + + listOptions := &client.ListOptions{ + LabelSelector: labels.NewSelector().Add(*hashReq, *siUIDReq), + } + + if err := r.deleteBindings(ctx, 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). @@ -369,9 +374,84 @@ 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 } + +// 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 + "-", + Labels: map[string]string{ + scopeInstanceUIDKey: string(in.GetUID()), + referenceHashKey: hashScopeInstanceAndTemplate(in, st), + clusterRoleBindingGenerateKey: cr.GenerateName, + }, + }, + Subjects: cr.Subjects, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: cr.GenerateName, + APIGroup: rbacv1.GroupName, + }, + } + + err := ctrl.SetControllerReference(in, crb, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } + return crb +} + +// 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 + "-", + Namespace: namespace, + Labels: map[string]string{ + scopeInstanceUIDKey: string(in.GetUID()), + referenceHashKey: hashScopeInstanceAndTemplate(in, st), + clusterRoleBindingGenerateKey: cr.GenerateName, + }, + }, + Subjects: cr.Subjects, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: cr.GenerateName, + APIGroup: rbacv1.GroupName, + }, + } + + err := ctrl.SetControllerReference(in, rb, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } + return rb +} + +// 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 { + 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) +} diff --git a/controllers/scopetemplate_controller.go b/controllers/scopetemplate_controller.go index be40e0b..39dc2dc 100644 --- a/controllers/scopetemplate_controller.go +++ b/controllers/scopetemplate_controller.go @@ -25,12 +25,15 @@ 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" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -46,6 +49,12 @@ type ScopeTemplateReconciler struct { } const ( + // scopeTemplateUIDKey is used to track "owners" of (Cluster)Roles we create. + scopeTemplateUIDKey = "operators.coreos.io/scopeTemplateUID" + + // 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 clusterRoleGenerateKey = "operators.coreos.io/generateName" stCtrlFieldOwner = "scopetemplate-controller" @@ -71,21 +80,38 @@ 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 { - return ctrl.Result{}, err + existingSt := &operatorsv1.ScopeTemplate{} + if err := r.Client.Get(ctx, req.NamespacedName, existingSt); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) } - scopeinstances := operatorsv1.ScopeInstanceList{} - - if err := r.Client.List(ctx, &scopeinstances, client.InNamespace(st.Namespace)); err != nil { - return ctrl.Result{}, err + 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 { @@ -95,26 +121,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 (Cluster)Roles + 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 (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 } - 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 } @@ -132,10 +163,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 } @@ -148,7 +177,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 @@ -156,26 +184,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.clusterRoleManifest(&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 { @@ -183,34 +195,32 @@ 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 { + // Create the ClusterRole if it does not exist + 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.V(2).Info("existing ClusterRole does not need to be updated", "UID", existingCR.GetUID()) return nil } - existingCRB.Labels = clusterRole.Labels - existingCRB.OwnerReferences = clusterRole.OwnerReferences - existingCRB.Rules = clusterRole.Rules + + patchObj := r.clusterRolePatchObj(existingCR, clusterRole) // server-side apply patch - existingCRB.ManagedFields = nil if err := r.Client.Patch(ctx, - existingCRB, + patchObj, client.Apply, client.FieldOwner(stCtrlFieldOwner), client.ForceOwnership); err != nil { @@ -220,6 +230,21 @@ func (r *ScopeTemplateReconciler) ensureClusterRoles(ctx context.Context, st *op return nil } +func (r *ScopeTemplateReconciler) clusterRolePatchObj(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, + }, + } +} + func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOptions ...client.ListOption) error { clusterRoles := &rbacv1.ClusterRoleList{} if err := r.Client.List(ctx, clusterRoles, listOptions...); err != nil { @@ -235,3 +260,23 @@ func (r *ScopeTemplateReconciler) deleteClusterRoles(ctx context.Context, listOp } return nil } + +func (r *ScopeTemplateReconciler) clusterRoleManifest(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, + } + + err := ctrl.SetControllerReference(st, cr, r.Scheme) + if err != nil { + log.Log.Error(err, "setting controller reference for ClusterRoleBinding") + } + return cr +} 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)) 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