fix(cn-controller): guard dereference while trying to clear resources

pkg/controller/sub_controller/be/controller_test.go

@@ -18,67 +18,40 @@ package be
 
 import (
 	"context"
-	"fmt"
+	"testing"
+
 	v1 "github.com/apache/doris-operator/api/doris/v1"
 	"github.com/apache/doris-operator/pkg/common/utils/resource"
 	appv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	k8sruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/client-go/rest"
-	"path/filepath"
-	"runtime"
-	ctrl "sigs.k8s.io/controller-runtime"
+	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/envtest"
-	"testing"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
-var cfg *rest.Config
-var testEnv *envtest.Environment
-var mgr ctrl.Manager
-
-func init() {
-	testEnv = &envtest.Environment{
-		Scheme: k8sruntime.NewScheme(),
-		BinaryAssetsDirectory: filepath.Join("..", "..", "..", "..", "bin", "k8s",
-			fmt.Sprintf("1.26.1-%s-%s", runtime.GOOS, runtime.GOARCH)),
-		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "..", "..", "config", "crd", "bases")},
-		ErrorIfCRDPathMissing: true,
-	}
-
-	var err error
-	cfg, err = testEnv.Start()
-	if err != nil {
-		fmt.Println("init failed, err=" + err.Error())
+func newBeController(t *testing.T, objects ...client.Object) (*Controller, client.Client) {
+	t.Helper()
+	scheme := runtime.NewScheme()
+	if err := appv1.AddToScheme(scheme); err != nil {
+		t.Fatalf("add appsv1 to scheme failed, err=%s", err.Error())
 	}
-	mgr, err = ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme})
-	if err != nil {
-		fmt.Println("Test_safeScaleDown NewManager failed, err=" + err.Error())
+	if err := corev1.AddToScheme(scheme); err != nil {
+		t.Fatalf("add corev1 to scheme failed, err=%s", err.Error())
 	}
-	go func() {
-		err = mgr.Start(ctrl.SetupSignalHandler())
-	}()
+	k8sclient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
+	return New(k8sclient, record.NewFakeRecorder(10)), k8sclient
 }
 
 func Test_ClearResources(t *testing.T) {
-	defer deferClear()
-	bc := New(mgr.GetClient(), mgr.GetEventRecorderFor("test-be"))
 	cluster := &v1.DorisCluster{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test",
 			Namespace: "default",
 		},
-		//Spec: v1.DorisClusterSpec{
-		//    BeSpec: &v1.BeSpec{
-		//        BaseSpec: v1.BaseSpec{
-		//            Replicas: resource.GetInt32Pointer(3),
-		//        },
-		//    },
-		//},
 		Status: v1.DorisClusterStatus{
 			BEStatus: &v1.ComponentStatus{
 				ComponentCondition: v1.ComponentCondition{
@@ -159,26 +132,16 @@ func Test_ClearResources(t *testing.T) {
 			},
 		}}
 
-	for _, dcrResource := range resources {
-		if err := mgr.GetClient().Create(context.Background(), dcrResource); err != nil {
-			t.Errorf("Test_ClearResources  create resource name =%s, failed,err=%s", dcrResource.GetName(), err.Error())
-		}
-	}
+	bc, k8sclient := newBeController(t, resources...)
 	if _, err := bc.ClearResources(context.Background(), cluster); err != nil {
 		t.Errorf("Test_ClearResources clear resources failed, err=%s", err.Error())
 	}
 
-	for i, _ := range resources {
+	for i := range resources {
 		name := resources[i].GetName()
-		if err := mgr.GetClient().Get(context.Background(), types.NamespacedName{Namespace: "default", Name: name}, resources[i]); err != nil && !apierrors.IsNotFound(err) {
-			t.Errorf("Test_ClearResources the resource is not deleted, name=%s, err=%s", name, err.Error())
+		err := k8sclient.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: name}, resources[i])
+		if !apierrors.IsNotFound(err) {
+			t.Errorf("Test_ClearResources the resource is not deleted, name=%s, err=%v", name, err)
 		}
 	}
 }
-
-func deferClear() {
-	err := testEnv.Stop()
-	if err != nil {
-		fmt.Println(err)
-	}
-}

pkg/controller/sub_controller/cn/controller.go

@@ -19,6 +19,8 @@ package cn
 
 import (
 	"context"
+	"time"
+
 	dorisv1 "github.com/apache/doris-operator/api/doris/v1"
 	"github.com/apache/doris-operator/pkg/common/utils"
 	"github.com/apache/doris-operator/pkg/common/utils/k8s"
@@ -31,7 +33,6 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"time"
 )
 
 type Controller struct {
@@ -223,7 +224,8 @@ func (cn *Controller) ClearResources(ctx context.Context, dcr *dorisv1.DorisClus
 	}
 
 	// clear autoscaler when autoscaler config deleted or the doriscluster deleted.
-	if dcr.Spec.CnSpec.AutoScalingPolicy == nil || !dcr.DeletionTimestamp.IsZero() {
+	// guard the dereference, only calls ClearResources when CnSpec is nil.
+	if dcr.Spec.CnSpec == nil || dcr.Spec.CnSpec.AutoScalingPolicy == nil || !dcr.DeletionTimestamp.IsZero() {
 		if err := cn.DeleteAutoscaler(ctx, dcr); err != nil {
 			cn.K8srecorder.Eventf(dcr, string(sub_controller.EventWarning), sub_controller.AutoScalerDeleteFailed, "cn autoscaler deleted failed."+err.Error())
 		}

pkg/controller/sub_controller/cn/controller_test.go

@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package cn
+
+import (
+	"context"
+	"testing"
+
+	v1 "github.com/apache/doris-operator/api/doris/v1"
+	appv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func newCnController(t *testing.T, objects ...client.Object) *Controller {
+	t.Helper()
+	scheme := runtime.NewScheme()
+	if err := appv1.AddToScheme(scheme); err != nil {
+		t.Fatalf("add appsv1 to scheme failed, err=%s", err.Error())
+	}
+	if err := corev1.AddToScheme(scheme); err != nil {
+		t.Fatalf("add corev1 to scheme failed, err=%s", err.Error())
+	}
+	return New(fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build(), record.NewFakeRecorder(10))
+}
+
+// Sync only calls ClearResources when CnSpec is nil, so a cluster whose cnSpec was removed while
+// status.cnStatus is still set must not dereference the spec.
+func Test_ClearResources(t *testing.T) {
+	cc := newCnController(t)
+	cluster := &v1.DorisCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "default",
+		},
+		Spec: v1.DorisClusterSpec{
+			CnSpec: nil,
+		},
+		Status: v1.DorisClusterStatus{
+			CnStatus: &v1.CnStatus{
+				ComponentStatus: v1.ComponentStatus{
+					ComponentCondition: v1.ComponentCondition{
+						Phase: v1.Reconciling,
+					},
+				},
+			},
+		},
+	}
+
+	cleared, err := cc.ClearResources(context.Background(), cluster)
+	if err != nil {
+		t.Errorf("Test_ClearResources clear resources failed, err=%s", err.Error())
+	}
+	if !cleared {
+		t.Error("Test_ClearResources expected resources to be cleared")
+	}
+}
+
+// A cluster that never had cn returns early on the nil status, before reaching the spec.
+func Test_ClearResources_NoCnStatus(t *testing.T) {
+	cc := newCnController(t)
+	cluster := &v1.DorisCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-no-cn",
+			Namespace: "default",
+		},
+	}
+
+	cleared, err := cc.ClearResources(context.Background(), cluster)
+	if err != nil {
+		t.Errorf("Test_ClearResources_NoCnStatus failed, err=%s", err.Error())
+	}
+	if !cleared {
+		t.Error("Test_ClearResources_NoCnStatus expected resources to be cleared")
+	}
+}