[incubator-uniffle] branch master updated: [Bug] Fix invalid owner of host path volumes (#323) (#330)

[ro...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

roryqi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git


The following commit(s) were added to refs/heads/master by this push:
     new 87361da6 [Bug] Fix invalid owner of host path volumes (#323) (#330)
87361da6 is described below

commit 87361da66a467fabbdbecf25061b8e2782c5d9ce
Author: jasonawang <ja...@tencent.com>
AuthorDate: Wed Nov 16 16:40:43 2022 +0800

    [Bug] Fix invalid owner of host path volumes (#323) (#330)
    
    ### What changes were proposed in this pull request?
    fix #323
    I have modified it for time reasons, thanks for pointing this out.
    I changed the owner of host paths as UID:GID.
    
    ### Why are the changes needed?
    Fix bug
    
    ### Does this PR introduce _any_ user-facing change?
    No
    
    ### How was this patch tested?
    UT
---
 .../pkg/controller/controller/process_rss_test.go  | 44 +++++-----
 .../operator/pkg/controller/util/util.go           | 14 ++-
 .../operator/pkg/controller/util/util_test.go      | 99 ++++++++++++++++++++++
 deploy/kubernetes/operator/pkg/utils/util.go       | 15 ----
 4 files changed, 134 insertions(+), 38 deletions(-)

diff --git a/deploy/kubernetes/operator/pkg/controller/controller/process_rss_test.go b/deploy/kubernetes/operator/pkg/controller/controller/process_rss_test.go
index a867bb62..a5ac8748 100644
--- a/deploy/kubernetes/operator/pkg/controller/controller/process_rss_test.go
+++ b/deploy/kubernetes/operator/pkg/controller/controller/process_rss_test.go
@@ -76,26 +76,28 @@ func TestProcessEmptyPhaseRss(t *testing.T) {
 			expectedNeedRetry: false,
 		},
 	} {
-		needRetry, err := rc.processNormal(rss)
-		if err != nil {
-			t.Errorf("process rss object failed: %v", err)
-			return
-		}
-		if needRetry != tt.expectedNeedRetry {
-			t.Errorf("unexpected result indicates whether to retrys: %v, expected: %v",
-				needRetry, tt.expectedNeedRetry)
-			return
-		}
-		updatedRss, getErr := rssClient.UniffleV1alpha1().RemoteShuffleServices(rss.Namespace).
-			Get(context.TODO(), rss.Name, metav1.GetOptions{})
-		if getErr != nil {
-			t.Errorf("get updated rss object failed: %v", err)
-			return
-		}
-		if !reflect.DeepEqual(updatedRss.Status, tt.expectedRssStatus) {
-			t.Errorf("unexpected status of updated rss object: %+v, expected: %+v",
-				updatedRss.Status, tt.expectedRssStatus)
-			return
-		}
+		t.Run(tt.name, func(tc *testing.T) {
+			needRetry, err := rc.processNormal(rss)
+			if err != nil {
+				tc.Errorf("process rss object failed: %v", err)
+				return
+			}
+			if needRetry != tt.expectedNeedRetry {
+				tc.Errorf("unexpected result indicates whether to retrys: %v, expected: %v",
+					needRetry, tt.expectedNeedRetry)
+				return
+			}
+			updatedRss, getErr := rssClient.UniffleV1alpha1().RemoteShuffleServices(rss.Namespace).
+				Get(context.TODO(), rss.Name, metav1.GetOptions{})
+			if getErr != nil {
+				tc.Errorf("get updated rss object failed: %v", err)
+				return
+			}
+			if !reflect.DeepEqual(updatedRss.Status, tt.expectedRssStatus) {
+				tc.Errorf("unexpected status of updated rss object: %+v, expected: %+v",
+					updatedRss.Status, tt.expectedRssStatus)
+				return
+			}
+		})
 	}
 }
diff --git a/deploy/kubernetes/operator/pkg/controller/util/util.go b/deploy/kubernetes/operator/pkg/controller/util/util.go
index 6f588681..bc1f3ccb 100644
--- a/deploy/kubernetes/operator/pkg/controller/util/util.go
+++ b/deploy/kubernetes/operator/pkg/controller/util/util.go
@@ -209,9 +209,19 @@ func generateLogVolumeMount(rssPodSpec *v1alpha1.RSSPodSpec) *corev1.VolumeMount
 
 func generateMakeDataDirCommand(rssPodSpec *v1alpha1.RSSPodSpec) []string {
 	var commands []string
-	fsGroup := *rssPodSpec.SecurityContext.FSGroup
+
+	var runAsUser int64
+	if rssPodSpec.SecurityContext != nil && rssPodSpec.SecurityContext.RunAsUser != nil {
+		runAsUser = *rssPodSpec.SecurityContext.RunAsUser
+	}
+
+	var fsGroup int64
+	if rssPodSpec.SecurityContext != nil && rssPodSpec.SecurityContext.FSGroup != nil {
+		fsGroup = *rssPodSpec.SecurityContext.FSGroup
+	}
+
 	for _, mountPath := range rssPodSpec.HostPathMounts {
-		commands = append(commands, fmt.Sprintf("chown -R %v:%v %v", fsGroup, fsGroup, mountPath))
+		commands = append(commands, fmt.Sprintf("chown -R %v:%v %v", runAsUser, fsGroup, mountPath))
 	}
 	return commands
 }
diff --git a/deploy/kubernetes/operator/pkg/controller/util/util_test.go b/deploy/kubernetes/operator/pkg/controller/util/util_test.go
new file mode 100644
index 00000000..5a49c5ef
--- /dev/null
+++ b/deploy/kubernetes/operator/pkg/controller/util/util_test.go
@@ -0,0 +1,99 @@
+/*
+ * 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 util
+
+import (
+	"sort"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/pointer"
+
+	"github.com/apache/incubator-uniffle/deploy/kubernetes/operator/api/uniffle/v1alpha1"
+)
+
+func TestGenerateMakeDataDirCommand(t *testing.T) {
+	for _, tt := range []struct {
+		name             string
+		rssPodSpec       *v1alpha1.RSSPodSpec
+		expectedCommands []string
+	}{
+		{
+			name: "empty security context",
+			rssPodSpec: &v1alpha1.RSSPodSpec{
+				HostPathMounts: map[string]string{
+					"/data1": "/mnt/data1",
+				},
+			},
+			expectedCommands: []string{
+				"chown -R 0:0 /mnt/data1",
+			},
+		},
+		{
+			name: "empty runAsUser field in security context",
+			rssPodSpec: &v1alpha1.RSSPodSpec{
+				HostPathMounts: map[string]string{
+					"/data2": "/mnt/data2",
+				},
+				SecurityContext: &corev1.PodSecurityContext{
+					FSGroup: pointer.Int64(1000),
+				},
+			},
+			expectedCommands: []string{
+				"chown -R 0:1000 /mnt/data2",
+			},
+		},
+		{
+			name: "non empty field of runAsUser and fsGroup in security context",
+			rssPodSpec: &v1alpha1.RSSPodSpec{
+				HostPathMounts: map[string]string{
+					"/data3": "/mnt/data3",
+				},
+				SecurityContext: &corev1.PodSecurityContext{
+					RunAsUser: pointer.Int64(2000),
+					FSGroup:   pointer.Int64(1000),
+				},
+			},
+			expectedCommands: []string{
+				"chown -R 2000:1000 /mnt/data3",
+			},
+		},
+	} {
+		t.Run(tt.name, func(tc *testing.T) {
+			commands := generateMakeDataDirCommand(tt.rssPodSpec)
+			if !isEqualStringSlice(commands, tt.expectedCommands) {
+				tc.Errorf("unexpected commands: %+v, expected: %+v", commands, tt.expectedCommands)
+				return
+			}
+		})
+	}
+}
+
+func isEqualStringSlice(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	sort.Strings(a)
+	sort.Strings(b)
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}
diff --git a/deploy/kubernetes/operator/pkg/utils/util.go b/deploy/kubernetes/operator/pkg/utils/util.go
index bd336f48..4c609441 100644
--- a/deploy/kubernetes/operator/pkg/utils/util.go
+++ b/deploy/kubernetes/operator/pkg/utils/util.go
@@ -26,7 +26,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
@@ -35,20 +34,6 @@ import (
 	"github.com/apache/incubator-uniffle/deploy/kubernetes/operator/pkg/constants"
 )
 
-// CreatePodInformerFactory creates pod informer factory by label selector.
-func CreatePodInformerFactory(kubeClient kubernetes.Interface,
-	key, value string) informers.SharedInformerFactory {
-	option := func(options *metav1.ListOptions) {
-		if len(value) > 0 {
-			options.LabelSelector = key + "=" + value
-		} else {
-			options.LabelSelector = key
-		}
-	}
-	return informers.NewSharedInformerFactoryWithOptions(kubeClient, 0,
-		informers.WithTweakListOptions(option))
-}
-
 // GetCurrentNamespace returns current namespace.
 func GetCurrentNamespace() string {
 	namespace := os.Getenv(constants.PodNamespaceEnv)