From 168f813a633c01081c25eaac7ce9d4a442fde1b8 Mon Sep 17 00:00:00 2001 From: Xuecheng Zhang Date: Tue, 26 Nov 2024 11:02:38 +0800 Subject: [PATCH] fix(TidbMonitor): do not set OwnerReferences for ClusterRole and ClusterRoleBinding (#5956) --- pkg/apis/label/label.go | 3 + pkg/controller/generic_control.go | 4 +- pkg/monitor/monitor/monitor_manager.go | 88 ++++++++++++++++++--- pkg/monitor/monitor/monitor_manager_test.go | 5 ++ pkg/monitor/monitor/util.go | 14 ++-- 5 files changed, 92 insertions(+), 22 deletions(-) diff --git a/pkg/apis/label/label.go b/pkg/apis/label/label.go index fd3b3e38236..42bad7ac9ae 100644 --- a/pkg/apis/label/label.go +++ b/pkg/apis/label/label.go @@ -192,6 +192,9 @@ const ( // TiDBMonitorVal is Monitor label value TiDBMonitorVal string = "monitor" + // TiDBMonitorProtectionFinalizer is the name of finalizer on TidbMonitors + TiDBMonitorProtectionFinalizer string = "tidb.pingcap.com/monitor-protection" + // CleanJobLabelVal is clean job label value CleanJobLabelVal string = "clean" // RestoreJobLabelVal is restore job label value diff --git a/pkg/controller/generic_control.go b/pkg/controller/generic_control.go index db491589f6e..44bb1984e52 100644 --- a/pkg/controller/generic_control.go +++ b/pkg/controller/generic_control.go @@ -103,7 +103,7 @@ func (w *typedWrapper) CreateOrUpdateClusterRoleBinding(controller client.Object existingCRB.RoleRef = desiredCRB.RoleRef existingCRB.Subjects = desiredCRB.Subjects return nil - }, true) + }, false) if err != nil { return nil, err } @@ -118,7 +118,7 @@ func (w *typedWrapper) CreateOrUpdateClusterRole(controller client.Object, clust existingCRole.Labels = desiredCRole.Labels existingCRole.Rules = desiredCRole.Rules return nil - }, true) + }, false) if err != nil { return nil, err } diff --git a/pkg/monitor/monitor/monitor_manager.go b/pkg/monitor/monitor/monitor_manager.go index 39472419661..2394c7ee624 100644 --- a/pkg/monitor/monitor/monitor_manager.go +++ b/pkg/monitor/monitor/monitor_manager.go @@ -14,11 +14,25 @@ package monitor import ( + "context" "encoding/json" "fmt" "sort" "strings" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbac "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" + "k8s.io/client-go/discovery" + discoverycachedmemory "k8s.io/client-go/discovery/cached/memory" + "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/pingcap/tidb-operator/pkg/apis/label" "github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1" v1alpha1validation "github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1/validation" @@ -31,17 +45,6 @@ import ( "github.com/pingcap/tidb-operator/pkg/pdapi" "github.com/pingcap/tidb-operator/pkg/util" utildiscovery "github.com/pingcap/tidb-operator/pkg/util/discovery" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - rbac "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/selection" - "k8s.io/client-go/discovery" - discoverycachedmemory "k8s.io/client-go/discovery/cached/memory" - "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/client" ) type MonitorManager struct { @@ -67,9 +70,13 @@ func NewMonitorManager(deps *controller.Dependencies) *MonitorManager { } func (m *MonitorManager) SyncMonitor(monitor *v1alpha1.TidbMonitor) error { + if err := m.addProtectionFinalizerIfNeed(monitor); err != nil { + return err + } if monitor.DeletionTimestamp != nil { - return nil + return m.cleanAndRemoveProtectionFinalizerIfNeed(monitor) } + if len(monitor.Spec.Clusters) < 1 && (monitor.Spec.DM == nil || len(monitor.Spec.DM.Clusters) < 1) { klog.Errorf("tm[%s/%s] does not configure the target tidbcluster", monitor.Namespace, monitor.Name) return nil @@ -187,6 +194,63 @@ func (m *MonitorManager) SyncMonitor(monitor *v1alpha1.TidbMonitor) error { return nil } +func (m *MonitorManager) addProtectionFinalizerIfNeed(tm *v1alpha1.TidbMonitor) error { + if tm.DeletionTimestamp != nil { + return nil + } + if !controllerutil.ContainsFinalizer(tm, label.TiDBMonitorProtectionFinalizer) { + controllerutil.AddFinalizer(tm, label.TiDBMonitorProtectionFinalizer) + _, err := m.deps.Clientset.PingcapV1alpha1().TidbMonitors(tm.Namespace).Update(context.TODO(), tm, metav1.UpdateOptions{}) + if err != nil { + return err + } + } + return nil +} + +func (m *MonitorManager) cleanAndRemoveProtectionFinalizerIfNeed(tm *v1alpha1.TidbMonitor) error { + if tm.DeletionTimestamp.IsZero() { + return nil + } + + if tm.Spec.ClusterScoped { + // we need to clean ClusterRole and ClusterRoleBinding as we can't set ownerReference for them + // so we call DELETE directly for them (but without waiting for the deletion to complete) + clusterRole, err := m.deps.KubeClientset.RbacV1().ClusterRoles().Get(context.TODO(), GetMonitorObjectNameCrossNamespace(tm), metav1.GetOptions{}) + if err != nil && !errors.IsNotFound(err) { + return err + } else if err == nil { + if clusterRole.Labels[label.ManagedByLabelKey] == label.TiDBOperator { + err = m.deps.KubeClientset.RbacV1().ClusterRoles().Delete(context.TODO(), clusterRole.Name, metav1.DeleteOptions{}) + if err != nil { + return err + } + } + } + + clusterRoleBinding, err := m.deps.KubeClientset.RbacV1().ClusterRoleBindings().Get(context.TODO(), GetMonitorObjectNameCrossNamespace(tm), metav1.GetOptions{}) + if err != nil && !errors.IsNotFound(err) { + return err + } else if err == nil { + if clusterRoleBinding.Labels[label.ManagedByLabelKey] == label.TiDBOperator { + err = m.deps.KubeClientset.RbacV1().ClusterRoleBindings().Delete(context.TODO(), clusterRoleBinding.Name, metav1.DeleteOptions{}) + if err != nil { + return err + } + } + } + } + + if controllerutil.ContainsFinalizer(tm, label.TiDBMonitorProtectionFinalizer) { + controllerutil.RemoveFinalizer(tm, label.TiDBMonitorProtectionFinalizer) + _, err := m.deps.Clientset.PingcapV1alpha1().TidbMonitors(tm.Namespace).Update(context.TODO(), tm, metav1.UpdateOptions{}) + if err != nil { + return err + } + } + return nil +} + func (m *MonitorManager) syncTidbMonitorStatus(monitor *v1alpha1.TidbMonitor) error { sts, err := m.deps.StatefulSetLister.StatefulSets(monitor.Namespace).Get(GetMonitorObjectName(monitor)) if err != nil { diff --git a/pkg/monitor/monitor/monitor_manager_test.go b/pkg/monitor/monitor/monitor_manager_test.go index 4cb88361265..e98038acd78 100644 --- a/pkg/monitor/monitor/monitor_manager_test.go +++ b/pkg/monitor/monitor/monitor_manager_test.go @@ -14,6 +14,7 @@ package monitor import ( + "context" "testing" "time" @@ -72,6 +73,8 @@ func TestTidbMonitorSyncCreate(t *testing.T) { if tm.Spec.Shards == nil { tm.Spec.Shards = pointer.Int32Ptr(0) } + _, err = tmm.deps.Clientset.PingcapV1alpha1().TidbMonitors(tm.Namespace).Create(context.Background(), tm, metav1.CreateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) err = tmm.SyncMonitor(tm) if test.errExpectFn != nil { @@ -548,6 +551,8 @@ func TestTidbMonitorSyncUpdate(t *testing.T) { if test.prepare != nil { test.prepare(tmm, tm) } + _, err = tmm.deps.Clientset.PingcapV1alpha1().TidbMonitors(tm.Namespace).Create(context.Background(), tm, metav1.CreateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) err = tmm.SyncMonitor(tm) diff --git a/pkg/monitor/monitor/util.go b/pkg/monitor/monitor/util.go index 10ca0499c9f..f06558d5e0f 100644 --- a/pkg/monitor/monitor/util.go +++ b/pkg/monitor/monitor/util.go @@ -253,10 +253,9 @@ func getMonitorRole(monitor *v1alpha1.TidbMonitor, policyRules []rbac.PolicyRule func getMonitorClusterRole(monitor *v1alpha1.TidbMonitor, policyRules []rbac.PolicyRule) *rbac.ClusterRole { return &rbac.ClusterRole{ ObjectMeta: meta.ObjectMeta{ - Name: GetMonitorObjectNameCrossNamespace(monitor), - Namespace: monitor.Namespace, - Labels: buildTidbMonitorLabel(monitor.Name), - OwnerReferences: []meta.OwnerReference{controller.GetTiDBMonitorOwnerRef(monitor)}, + Name: GetMonitorObjectNameCrossNamespace(monitor), + Namespace: monitor.Namespace, + Labels: buildTidbMonitorLabel(monitor.Name), }, Rules: policyRules, } @@ -265,10 +264,9 @@ func getMonitorClusterRole(monitor *v1alpha1.TidbMonitor, policyRules []rbac.Pol func getMonitorClusterRoleBinding(sa *core.ServiceAccount, role *rbac.ClusterRole, monitor *v1alpha1.TidbMonitor) *rbac.ClusterRoleBinding { return &rbac.ClusterRoleBinding{ ObjectMeta: meta.ObjectMeta{ - Name: GetMonitorObjectNameCrossNamespace(monitor), - Namespace: monitor.Namespace, - Labels: buildTidbMonitorLabel(monitor.Name), - OwnerReferences: []meta.OwnerReference{controller.GetTiDBMonitorOwnerRef(monitor)}, + Name: GetMonitorObjectNameCrossNamespace(monitor), + Namespace: monitor.Namespace, + Labels: buildTidbMonitorLabel(monitor.Name), }, Subjects: []rbac.Subject{ {