Repository: kubean-io/kubean
Pull request #997 (2023-11-08 07:37:04)

add unit-test
<!--  

Thanks for sending a pull request!  Here are some tips for you:

* make sure your commit is signed off

-->

#### What type of PR is this?

<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind enhancement
/kind feature

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind design
/kind regression
-->

#### What this PR does / why we need it:

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer:



Patch:
diff --git a/artifacts/generate_offline_package.sh b/artifacts/generate_offline_package.sh
index a5dbbb964..38341dccd 100644
--- a/artifacts/generate_offline_package.sh
+++ b/artifacts/generate_offline_package.sh
@@ -109,15 +109,19 @@ function create_images() {
   fi
 
   while read -r image_name; do
-    image_name=$(replace_image_name "$image_name")
-    echo "download image $image_name to local"
+    ## quay.io/metallb/controller:v0.12.1 => dir:somedir/metallb%controller:v0.12.1
+    ## quay.io/metallb/controller:v0.12.1 => dir:somedir/quay.io%metallb%controller:v0.12.1 ## keep host with multi harbor projects
+
+    ## new_dir_name=${image_name#*/}     ## remote host
+    new_dir_name=${image_name} ## keep host
+    new_dir_name=${new_dir_name//\//%} ## replace all / with %
+    echo "download image $(replace_image_name "$image_name") to local $new_dir_name"
     ret=0
-    skopeo copy --insecure-policy --retry-times=3 --override-os linux --override-arch ${ARCH} "docker://$image_name" "oci:offline-images:$image_name" || ret=$?
+    skopeo copy --insecure-policy --retry-times=3 --override-os linux --override-arch ${ARCH} docker://"$(replace_image_name "$image_name")" dir:offline-images/"$new_dir_name" || ret=$?
     if [ ${ret} -ne 0 ]; then
       echo "skopeo copy image failed, image name: ${image_name}."
       exit 1
     fi
-    echo "$image_name" >> offline-images/images.list
   done <<< "$images_list_content"
 
   tar -czvf $OFFLINE_IMAGES_DIR/offline-images.tar.gz offline-images
diff --git a/artifacts/import_images.sh b/artifacts/import_images.sh
index 1c54420ca..4ad089a90 100755
--- a/artifacts/import_images.sh
+++ b/artifacts/import_images.sh
@@ -21,14 +21,14 @@ function check_local_image_files() {
   if [ $(ls -A offline-images | wc -l) -eq 0 ]; then
     return
   fi
-  while read -r image_name; do
-    if skopeo inspect "oci:offline-images:$image_name" >/dev/null 2>&1; then
-      echo "check $image_name successfully"
+  for dir_name in offline-images/*; do
+    if skopeo inspect dir:"$dir_name" >/dev/null 2>&1; then
+      echo "check $dir_name successfully"
     else
-      echo "$image_name maybe bad image file"
+      echo "check $dir_name and $dir_name maybe bad image file"
       exit 1
     fi
-  done < offline-images/images.list
+  done
 }
 
 function import_images() {
@@ -39,7 +39,7 @@ function import_images() {
 
   check_local_image_files
 
-  while read -r image_name; do
+  for dir_name in offline-images/*; do
     copy_cmd="skopeo copy --insecure-policy -a "
     if [ "$DEST_TLS_VERIFY" = false ]; then
       copy_cmd=$copy_cmd" --dest-tls-verify=false "
@@ -47,12 +47,15 @@ function import_images() {
     if [ -n "$DEST_USER" ]; then
       copy_cmd=$copy_cmd" --dest-creds=$DEST_USER:$DEST_PASS "
     fi
-    target_image_name="$REGISTRY_ADDR"/"$image_name"
+    ## dir:offline-images/abc.com%coreos%etcd:v3.5.4 docker://1.2.3.4:5000/abc.com/coreos/etcd:v3.5.4
+    image_name=${dir_name#*/}      # remove dir prefix
+    image_name=${image_name//%/\/} # replace % with /
+    image_name="$REGISTRY_ADDR"/"$image_name"
 
-    echo "import $image_name to $target_image_name"
+    echo "import $dir_name to $image_name"
 
-    $copy_cmd --retry-times=3 "oci:offline-images:$image_name" "docker://$target_image_name"
-  done < offline-images/images.list
+    $copy_cmd --retry-times=3 dir:"$dir_name" docker://"$image_name"
+  done
 
   echo "import completed!"
 }
diff --git a/pkg/webhooks/clusterops/clusterops_webhook.go b/pkg/webhooks/clusterops/clusterops_webhook.go
index d512ec11e..06cce7ee2 100644
--- a/pkg/webhooks/clusterops/clusterops_webhook.go
+++ b/pkg/webhooks/clusterops/clusterops_webhook.go
@@ -81,7 +81,7 @@ func CreateHTTPSCASecretWithLock(ctx context.Context, client kubernetes.Interfac
 		Callbacks: leaderelection.LeaderCallbacks{
 			OnStartedLeading: func(ctx context.Context) {
 				klog.Warningf("webhook create CA OnStartedLeading on %s", util.GetCurrentRunningPodName())
-				if err := EnsureCASecretExist(client); err == nil {
+				if err := EnsureCASecretExist(client, createHTTPSCAInSecret); err == nil {
 					UpdateClusterOperationWebhook(client)
 				}
 				<-ctx.Done()
@@ -107,7 +107,7 @@ func WaitForCASecretExist(client kubernetes.Interface) *corev1.Secret {
 	}
 }
 
-func EnsureCASecretExist(client kubernetes.Interface) error {
+func EnsureCASecretExist(client kubernetes.Interface, createHTTPSCAInSecret func() (*corev1.Secret, error)) error {
 	_, err := client.CoreV1().Secrets(util.GetCurrentNSOrDefault()).Get(context.Background(), CAStoreSecret, metav1.GetOptions{})
 	if err != nil && apierrors.IsNotFound(err) {
 		newSecret, err := createHTTPSCAInSecret()


Test patch:
diff --git a/pkg/controllers/clusterops/controller_test.go b/pkg/controllers/clusterops/controller_test.go
index 7bfca8935..eb16943b2 100644
--- a/pkg/controllers/clusterops/controller_test.go
+++ b/pkg/controllers/clusterops/controller_test.go
@@ -717,6 +717,60 @@ func TestIsValidImageName(t *testing.T) {
 	}
 }
 
+func TestUpdateStatusForLabel(t *testing.T) {
+	controller := Controller{
+		Client:                newFakeClient(),
+		ClientSet:             clientsetfake.NewSimpleClientset(),
+		KubeanClusterSet:      clusterv1alpha1fake.NewSimpleClientset(),
+		KubeanClusterOpsSet:   clusteroperationv1alpha1fake.NewSimpleClientset(),
+		InfoManifestClientSet: manifestv1alpha1fake.NewSimpleClientset(),
+	}
+	tests := []struct {
+		name string
+		args func() bool
+		want bool
+	}{
+		{
+			name: "nil labels",
+			args: func() bool {
+				ops := &clusteroperationv1alpha1.ClusterOperation{}
+				err := controller.UpdateStatusForLabel(ops)
+				return err == nil && ops.Labels != nil
+			},
+			want: true,
+		},
+		{
+			name: "already done",
+			args: func() bool {
+				ops := &clusteroperationv1alpha1.ClusterOperation{}
+				ops.Labels = map[string]string{constants.KubeanClusterHasCompleted: "done"}
+				err := controller.UpdateStatusForLabel(ops)
+				return err == nil
+			},
+			want: true,
+		},
+		{
+			name: "SucceededStatus",
+			args: func() bool {
+				ops := &clusteroperationv1alpha1.ClusterOperation{}
+				ops.Name = "test1"
+				controller.Client.Create(context.Background(), ops)
+				ops.Status.Status = clusteroperationv1alpha1.SucceededStatus
+				err := controller.UpdateStatusForLabel(ops)
+				return err == nil
+			},
+			want: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if test.args() != test.want {
+				t.Fatal()
+			}
+		})
+	}
+}
+
 func TestGetServiceAccountName(t *testing.T) {
 	controller := Controller{
 		Client:                newFakeClient(),
diff --git a/pkg/controllers/offlineversion/controller_test.go b/pkg/controllers/offlineversion/controller_test.go
index 9fcf36e08..438788d4a 100644
--- a/pkg/controllers/offlineversion/controller_test.go
+++ b/pkg/controllers/offlineversion/controller_test.go
@@ -5,7 +5,9 @@ package offlineversion
 
 import (
 	"context"
+	"fmt"
 	"net/http"
+	"reflect"
 	"testing"
 	"time"
 
@@ -17,6 +19,7 @@ import (
 	clientsetfake "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
+	k8stesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/record"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -47,6 +50,27 @@ func newFakeClient() client.Client {
 	return client
 }
 
+func fetchTestingFake(obj interface{ RESTClient() rest.Interface }) *k8stesting.Fake {
+	// https://stackoverflow.com/questions/69740891/mocking-errors-with-client-go-fake-client
+	return reflect.Indirect(reflect.ValueOf(obj)).FieldByName("Fake").Interface().(*k8stesting.Fake)
+}
+
+func removeReactorFromTestingTake(obj interface{ RESTClient() rest.Interface }, verb, resource string) {
+	if fakeObj := fetchTestingFake(obj); fakeObj != nil {
+		newReactionChain := make([]k8stesting.Reactor, 0)
+		fakeObj.Lock()
+		defer fakeObj.Unlock()
+		for i := range fakeObj.ReactionChain {
+			reaction := fakeObj.ReactionChain[i]
+			if simpleReaction, ok := reaction.(*k8stesting.SimpleReactor); ok && simpleReaction.Verb == verb && simpleReaction.Resource == resource {
+				continue // ignore
+			}
+			newReactionChain = append(newReactionChain, reaction)
+		}
+		fakeObj.ReactionChain = newReactionChain
+	}
+}
+
 func TestMergeOfflineVersion(t *testing.T) {
 	controller := &Controller{
 		Client:                    newFakeClient(),
@@ -319,6 +343,55 @@ func TestReconcile(t *testing.T) {
 			},
 			want: true,
 		},
+		{
+			name: "update global-manifest unsuccessfully",
+			args: func() bool {
+				controller := &Controller{
+					Client:                    newFakeClient(),
+					ClientSet:                 clientsetfake.NewSimpleClientset(),
+					LocalArtifactSetClientSet: localartifactsetv1alpha1fake.NewSimpleClientset(),
+					InfoManifestClientSet:     manifestv1alpha1fake.NewSimpleClientset(),
+				}
+				offlineVersionData := localartifactsetv1alpha1.LocalArtifactSet{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       "LocalArtifactSet",
+						APIVersion: "kubean.io/v1alpha1",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "offlineversion-1",
+					},
+					Spec: localartifactsetv1alpha1.Spec{
+						Docker: []*localartifactsetv1alpha1.DockerInfo{
+							{
+								OS:           "redhat-7",
+								VersionRange: []string{"20.1", "20.2"},
+							},
+						},
+					},
+				}
+
+				globalComponentsVersion := manifestv1alpha1.Manifest{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       "kubeanclusterconfig",
+						APIVersion: "kubean.io/v1alpha1",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name: constants.InfoManifestGlobal,
+					},
+				}
+
+				controller.Client.Create(context.Background(), &offlineVersionData)
+				controller.LocalArtifactSetClientSet.KubeanV1alpha1().LocalArtifactSets().Create(context.Background(), &offlineVersionData, metav1.CreateOptions{})
+				controller.InfoManifestClientSet.KubeanV1alpha1().Manifests().Create(context.Background(), &globalComponentsVersion, metav1.CreateOptions{})
+				fetchTestingFake(controller.InfoManifestClientSet.KubeanV1alpha1()).PrependReactor("update", "manifests/status", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, fmt.Errorf("this is error when updateStatus")
+				})
+				defer removeReactorFromTestingTake(controller.InfoManifestClientSet.KubeanV1alpha1(), "update", "manifests/status")
+				_, err := controller.Reconcile(context.Background(), controllerruntime.Request{NamespacedName: types.NamespacedName{Name: offlineVersionData.Name}})
+				return err == nil
+			},
+			want: true,
+		},
 		{
 			name: "ComponentsversionGlobal not exist",
 			args: func() bool {
diff --git a/pkg/util/cluster_test.go b/pkg/util/cluster_test.go
index cc0663b85..18895d0ff 100644
--- a/pkg/util/cluster_test.go
+++ b/pkg/util/cluster_test.go
@@ -134,6 +134,38 @@ func TestUpdateOwnReference(t *testing.T) {
 		args func() bool
 		want bool
 	}{
+		{
+			name: "already ownerReferences exist",
+			args: func() bool {
+				fakeClient := clientsetfake.NewSimpleClientset()
+				configMapList := []*apis.ConfigMapRef{{Name: "abc", NameSpace: "abc"}}
+				fakeClient.CoreV1().ConfigMaps(configMapList[0].NameSpace).Create(context.Background(), &corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            configMapList[0].Name,
+						OwnerReferences: []metav1.OwnerReference{{}},
+					},
+				}, metav1.CreateOptions{})
+				secretList := []*apis.SecretRef{{Name: "cba", NameSpace: "cba"}}
+				fakeClient.CoreV1().Secrets(secretList[0].NameSpace).Create(context.Background(), &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            secretList[0].Name,
+						OwnerReferences: []metav1.OwnerReference{{}},
+					},
+				}, metav1.CreateOptions{})
+				return UpdateOwnReference(fakeClient, configMapList, secretList, metav1.OwnerReference{}) == nil
+			},
+			want: true,
+		},
+		{
+			name: "empty RefData",
+			args: func() bool {
+				fakeClient := clientsetfake.NewSimpleClientset()
+				configMapList := []*apis.ConfigMapRef{{Name: "", NameSpace: ""}}
+				secretList := []*apis.SecretRef{{Name: "", NameSpace: ""}}
+				return UpdateOwnReference(fakeClient, configMapList, secretList, metav1.OwnerReference{}) == nil
+			},
+			want: true,
+		},
 		{
 			name: "not found",
 			args: func() bool {
diff --git a/pkg/webhooks/clusterops/clusterops_webhook_test.go b/pkg/webhooks/clusterops/clusterops_webhook_test.go
index 3b832df0c..3ae0c0f1e 100644
--- a/pkg/webhooks/clusterops/clusterops_webhook_test.go
+++ b/pkg/webhooks/clusterops/clusterops_webhook_test.go
@@ -183,11 +183,22 @@ func TestEnsureCASecretExist(t *testing.T) {
 		args func() bool
 		want bool
 	}{
+		{
+			name: "create certs unsuccessfully",
+			args: func() bool {
+				fakeClientSet := clientsetfake.NewSimpleClientset()
+				err := EnsureCASecretExist(fakeClientSet, func() (*corev1.Secret, error) {
+					return nil, fmt.Errorf("failed")
+				})
+				return err != nil
+			},
+			want: true,
+		},
 		{
 			name: "good case",
 			args: func() bool {
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				err := EnsureCASecretExist(fakeClientSet)
+				err := EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				data, _ := fakeClientSet.CoreV1().Secrets(util.GetCurrentNSOrDefault()).Get(context.Background(), CAStoreSecret, metav1.GetOptions{})
 				return err == nil && len(data.Data) == 2
 			},
@@ -201,7 +212,7 @@ func TestEnsureCASecretExist(t *testing.T) {
 					return true, nil, fmt.Errorf("create secret but error")
 				})
 				defer removeReactorFromTestingTake(fakeClientSet.CoreV1(), "create", "secrets")
-				err := EnsureCASecretExist(fakeClientSet)
+				err := EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				return err != nil && strings.Contains(err.Error(), "create secret but error")
 			},
 			want: true,
@@ -230,7 +241,7 @@ func TestUpdateClusterOperationWebhook(t *testing.T) {
 				ClusterOperationWebhook = "my-webhook-abc"
 				CreateHTTPSCAFilesFromSecret(&corev1.Secret{})
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				UpdateClusterOperationWebhook(fakeClientSet)
 				os.Remove(filepath.Join(certsDir, certFile))
 				CreateHTTPSCAFilesFromSecret(&corev1.Secret{})
@@ -259,7 +270,7 @@ func TestUpdateClusterOperationWebhook(t *testing.T) {
 			arg: func() bool {
 				certsDir = strings.TrimRight(os.TempDir(), "/")
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				fetchTestingFake(fakeClientSet.CoreV1()).PrependReactor("get", "secrets", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
 					return true, nil, fmt.Errorf("this is error")
 				})
@@ -274,7 +285,7 @@ func TestUpdateClusterOperationWebhook(t *testing.T) {
 			arg: func() bool {
 				certsDir = strings.TrimRight(os.TempDir(), "/")
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				fetchTestingFake(fakeClientSet.AdmissionregistrationV1()).PrependReactor("create", "validatingwebhookconfigurations", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
 					return true, nil, fmt.Errorf("create validatingwebhookconfigurations but error")
 				})
@@ -289,14 +300,14 @@ func TestUpdateClusterOperationWebhook(t *testing.T) {
 			arg: func() bool {
 				certsDir = strings.TrimRight(os.TempDir(), "/")
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				fetchTestingFake(fakeClientSet.AdmissionregistrationV1()).PrependReactor("update", "validatingwebhookconfigurations", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
 					return true, nil, fmt.Errorf("update validatingwebhookconfigurations but error")
 				})
 				defer removeReactorFromTestingTake(fakeClientSet.AdmissionregistrationV1(), "update", "validatingwebhookconfigurations")
 				UpdateClusterOperationWebhook(fakeClientSet)
 				fakeClientSet.CoreV1().Secrets(util.GetCurrentNSOrDefault()).Delete(context.Background(), CAStoreSecret, metav1.DeleteOptions{})
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				err := UpdateClusterOperationWebhook(fakeClientSet)
 				return err != nil && err.Error() == "update validatingwebhookconfigurations but error"
 			},
@@ -307,7 +318,7 @@ func TestUpdateClusterOperationWebhook(t *testing.T) {
 			arg: func() bool {
 				certsDir = strings.TrimRight(os.TempDir(), "/")
 				fakeClientSet := clientsetfake.NewSimpleClientset()
-				EnsureCASecretExist(fakeClientSet)
+				EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				fetchTestingFake(fakeClientSet.AdmissionregistrationV1()).PrependReactor("update", "validatingwebhookconfigurations", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
 					return true, nil, fmt.Errorf("update validatingwebhookconfigurations but error")
 				})
@@ -537,7 +548,7 @@ func TestWaitForCASecretExist(t *testing.T) {
 				fakeClientSet := clientsetfake.NewSimpleClientset()
 				go func() {
 					time.Sleep(time.Second)
-					EnsureCASecretExist(fakeClientSet)
+					EnsureCASecretExist(fakeClientSet, createHTTPSCAInSecret)
 				}()
 				data := WaitForCASecretExist(fakeClientSet)
 				return data != nil
@@ -574,6 +585,16 @@ func TestCreateHTTPSCASecretWithLock(t *testing.T) {
 			},
 			want: true,
 		},
+		{
+			name: "error case with nil",
+			args: func() bool {
+				ctx, cancel := context.WithCancel(context.Background())
+				defer cancel()
+				err := CreateHTTPSCASecretWithLock(ctx, nil)
+				return err == nil
+			},
+			want: true,
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {