Merge pull request #1047 from stashed/rm-fsgroup

Remove defaulting FSGroup

pkg/controller/backup_configuration.go

@@ -255,22 +255,39 @@ func (c *StashController) EnsureBackupTriggeringCronJob(invoker apis.Invoker) er
 		// ensure that job gets deleted on completion
 		in.Spec.JobTemplate.Labels[apis.KeyDeleteJobOnCompletion] = apis.AllowDeletingJobOnCompletion
 
+		container := core.Container{
+			Name:            apis.StashContainer,
+			ImagePullPolicy: core.PullIfNotPresent,
+			Image:           image.ToContainerImage(),
+			Args: []string{
+				"create-backupsession",
+				fmt.Sprintf("--invoker-name=%s", invoker.OwnerRef.Name),
+				fmt.Sprintf("--invoker-type=%s", invoker.OwnerRef.Kind),
+			},
+		}
+		// only apply the container level runtime settings that make sense for the CronJob
+		if invoker.RuntimeSettings.Container != nil {
+			container.Resources = invoker.RuntimeSettings.Container.Resources
+			container.Env = invoker.RuntimeSettings.Container.Env
+			container.EnvFrom = invoker.RuntimeSettings.Container.EnvFrom
+			container.SecurityContext = invoker.RuntimeSettings.Container.SecurityContext
+		}
+
 		in.Spec.JobTemplate.Spec.Template.Spec.Containers = core_util.UpsertContainer(
-			in.Spec.JobTemplate.Spec.Template.Spec.Containers,
-			core.Container{
-				Name:            apis.StashContainer,
-				ImagePullPolicy: core.PullIfNotPresent,
-				Image:           image.ToContainerImage(),
-				Args: []string{
-					"create-backupsession",
-					fmt.Sprintf("--invoker-name=%s", invoker.OwnerRef.Name),
-					fmt.Sprintf("--invoker-type=%s", invoker.OwnerRef.Kind),
-				},
-			})
+			in.Spec.JobTemplate.Spec.Template.Spec.Containers, container)
 		in.Spec.JobTemplate.Spec.Template.Spec.RestartPolicy = core.RestartPolicyNever
 		in.Spec.JobTemplate.Spec.Template.Spec.ServiceAccountName = serviceAccountName
-		// insert default pod level security context
-		in.Spec.JobTemplate.Spec.Template.Spec.SecurityContext = util.UpsertDefaultPodSecurityContext(in.Spec.JobTemplate.Spec.Template.Spec.SecurityContext)
+
+		// only apply the pod level runtime settings that make sense for the CronJob
+		if invoker.RuntimeSettings.Pod != nil {
+			if len(invoker.RuntimeSettings.Pod.ImagePullSecrets) != 0 {
+				in.Spec.JobTemplate.Spec.Template.Spec.ImagePullSecrets = invoker.RuntimeSettings.Pod.ImagePullSecrets
+			}
+			if invoker.RuntimeSettings.Pod.SecurityContext != nil {
+				in.Spec.JobTemplate.Spec.Template.Spec.SecurityContext = invoker.RuntimeSettings.Pod.SecurityContext
+			}
+		}
+
 		return in
 	})
 

pkg/controller/init_container.go

@@ -88,20 +88,6 @@ func (c *StashController) ensureRestoreInitContainer(w *wapi.Workload, rs *api_v
 		util.NewRestoreInitContainer(rs, repository, image),
 	)
 
-	if rs.Spec.RuntimeSettings.Pod != nil {
-		// keep existing image pull secrets and add new image pull secrets if specified in RestoreSession spec.
-		w.Spec.Template.Spec.ImagePullSecrets = core_util.MergeLocalObjectReferences(
-			w.Spec.Template.Spec.ImagePullSecrets,
-			rs.Spec.RuntimeSettings.Pod.ImagePullSecrets,
-		)
-	}
-
-	// TODO: should we modify user's workloads security context?
-	// apply default pod level security context (fsGroup: 65535)
-	// this will not overwrite user provided security context
-	// it will just insert if not present.
-	w.Spec.Template.Spec.SecurityContext = util.UpsertDefaultPodSecurityContext(w.Spec.Template.Spec.SecurityContext)
-
 	// add an emptyDir volume for holding temporary files
 	w.Spec.Template.Spec.Volumes = util.UpsertTmpVolume(w.Spec.Template.Spec.Volumes, rs.Spec.TempDir)
 	// add  downward volume to make some information of the workload accessible to the container

pkg/controller/sidecar.go

@@ -200,20 +200,6 @@ func (c *StashController) ensureBackupSidecar(w *wapi.Workload, invoker apis.Inv
 		util.NewBackupSidecarContainer(invoker, targetInfo, &repository.Spec.Backend, image),
 	)
 
-	// keep existing image pull secrets
-	if targetInfo.RuntimeSettings.Pod != nil {
-		w.Spec.Template.Spec.ImagePullSecrets = core_util.MergeLocalObjectReferences(
-			w.Spec.Template.Spec.ImagePullSecrets,
-			targetInfo.RuntimeSettings.Pod.ImagePullSecrets,
-		)
-	}
-
-	// TODO: should we modify user's workloads security context?
-	// apply default pod level security context (fsGroup: 65535)
-	// this will not overwrite user provided security context
-	// it will just insert if not present.
-	w.Spec.Template.Spec.SecurityContext = util.UpsertDefaultPodSecurityContext(w.Spec.Template.Spec.SecurityContext)
-
 	w.Spec.Template.Spec.Volumes = util.UpsertTmpVolume(w.Spec.Template.Spec.Volumes, targetInfo.TempDir)
 	w.Spec.Template.Spec.Volumes = util.UpsertDownwardVolume(w.Spec.Template.Spec.Volumes)
 	w.Spec.Template.Spec.Volumes = util.UpsertSecretVolume(w.Spec.Template.Spec.Volumes, repository.Spec.Backend.StorageSecretName)

pkg/resolve/task.go

@@ -167,9 +167,6 @@ func (o TaskResolver) GetPodSpec(invokerType, invokerName, targetKind, targetNam
 		Containers:     containers[len(containers)-1:],
 		RestartPolicy:  core.RestartPolicyNever, // TODO: use OnFailure ?
 	}
-	// apply default pod level security context.
-	// don't overwrite user provided sc.
-	podSpec.SecurityContext = util.UpsertDefaultPodSecurityContext(podSpec.SecurityContext)
 
 	// apply RuntimeSettings to PodSpec
 	if o.RuntimeSettings.Pod != nil {

pkg/util/job.go

@@ -301,9 +301,6 @@ func NewPVCRestorerJob(rs *api_v1beta1.RestoreSession, repository *api_v1alpha1.
 		},
 	}
 
-	// Upsert default pod level security context
-	jobTemplate.Spec.SecurityContext = UpsertDefaultPodSecurityContext(jobTemplate.Spec.SecurityContext)
-
 	// Pass pod RuntimeSettings from RestoreSession
 	if rs.Spec.RuntimeSettings.Pod != nil {
 		jobTemplate.Spec = ofst_util.ApplyPodRuntimeSettings(jobTemplate.Spec, *rs.Spec.RuntimeSettings.Pod)
@@ -357,10 +354,6 @@ func NewVolumeSnapshotterJob(bs *api_v1beta1.BackupSession, backupTarget *api_v1
 		},
 	}
 
-	// apply default pod level security context
-	// don't overwrite user provided sc
-	jobTemplate.Spec.SecurityContext = UpsertDefaultPodSecurityContext(jobTemplate.Spec.SecurityContext)
-
 	// Pass pod RuntimeSettings from RestoreSession
 	if runtimeSettings.Pod != nil {
 		jobTemplate.Spec = ofst_util.ApplyPodRuntimeSettings(jobTemplate.Spec, *runtimeSettings.Pod)
@@ -404,10 +397,6 @@ func NewVolumeRestorerJob(rs *api_v1beta1.RestoreSession, image docker.Docker) (
 		},
 	}
 
-	// apply default pod level security context
-	// don't overwrite user provided sc
-	jobTemplate.Spec.SecurityContext = UpsertDefaultPodSecurityContext(jobTemplate.Spec.SecurityContext)
-
 	// Pass pod RuntimeSettings from RestoreSession
 	if rs.Spec.RuntimeSettings.Pod != nil {
 		jobTemplate.Spec = ofst_util.ApplyPodRuntimeSettings(jobTemplate.Spec, *rs.Spec.RuntimeSettings.Pod)

pkg/util/kubernetes.go

@@ -210,20 +210,6 @@ func UpsertPodSecurityContext(currentSC, newSC *core.PodSecurityContext) *core.P
 	return finalSC
 }
 
-func UpsertDefaultPodSecurityContext(currentSC *core.PodSecurityContext) *core.PodSecurityContext {
-
-	defaultSecurityContext := &core.PodSecurityContext{
-		// In GKE alpha clusters, service account token is only redable by owner or group
-		// xref: https://kubernetes.slack.com/archives/C09R1TL6A/p1560290949126300
-		FSGroup: types.Int64P(65535),
-	}
-	// Don't overwrite user provided one.
-	// First parameter is overwritten by second parameter.
-	// Hence, we are sending defaultSecurityContext as first parameter and currentSc as second parameter
-	// so that current one does not get overwritten by default one.
-	return UpsertPodSecurityContext(defaultSecurityContext, currentSC)
-}
-
 func MergeLocalVolume(volumes []core.Volume, backend *store.Backend) []core.Volume {
 	// check if stash-local volume already exist
 	oldPos := -1

test/e2e/framework/backup_configuration.go

@@ -17,6 +17,7 @@ limitations under the License.
 package framework
 
 import (
+	"strconv"
 	"strings"
 	"time"
 
@@ -26,6 +27,8 @@ import (
 
 	"github.com/appscode/go/crypto/rand"
 	. "github.com/onsi/gomega"
+	batchv1 "k8s.io/api/batch/v1"
+	batch_v1beta1 "k8s.io/api/batch/v1beta1"
 	core "k8s.io/api/core/v1"
 	kerr "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -79,7 +82,7 @@ func (fi *Invocation) DeleteBackupConfiguration(backupCfg v1beta1.BackupConfigur
 func (f *Framework) EventuallyCronJobCreated(meta metav1.ObjectMeta) GomegaAsyncAssertion {
 	return Eventually(
 		func() bool {
-			_, err := f.KubeClient.BatchV1beta1().CronJobs(meta.Namespace).Get(getBackupCronJobName(meta), metav1.GetOptions{})
+			_, err := f.GetCronJob(meta)
 			if err == nil && !kerr.IsNotFound(err) {
 				return true
 			}
@@ -90,6 +93,10 @@ func (f *Framework) EventuallyCronJobCreated(meta metav1.ObjectMeta) GomegaAsync
 	)
 }
 
+func (f *Framework) GetCronJob(meta metav1.ObjectMeta) (*batch_v1beta1.CronJob, error) {
+	return f.KubeClient.BatchV1beta1().CronJobs(meta.Namespace).Get(getBackupCronJobName(meta), metav1.GetOptions{})
+}
+
 func (f *Framework) EventuallyCronJobSuspended(meta metav1.ObjectMeta) GomegaAsyncAssertion {
 	return Eventually(
 		func() bool {
@@ -132,6 +139,14 @@ func (f *Framework) EventuallyBackupConfigurationCreated(meta metav1.ObjectMeta)
 	)
 }
 
+func (f *Framework) GetBackupJob(backupSessionName string) (*batchv1.Job, error) {
+	return f.KubeClient.BatchV1().Jobs(f.namespace).Get(getBackupJobName(backupSessionName, strconv.Itoa(0)), metav1.GetOptions{})
+}
+
 func getBackupCronJobName(objMeta metav1.ObjectMeta) string {
 	return meta_util.ValidCronJobNameWithPrefix(apis.PrefixStashBackup, strings.ReplaceAll(objMeta.Name, ".", "-"))
 }
+
+func getBackupJobName(backupSessionName string, index string) string {
+	return meta_util.ValidNameWithPefixNSuffix(apis.PrefixStashBackup, strings.ReplaceAll(backupSessionName, ".", "-"), index)
+}

test/e2e/framework/restore_session.go

@@ -17,15 +17,19 @@ limitations under the License.
 package framework
 
 import (
+	"strings"
 	"time"
 
+	"stash.appscode.dev/stash/apis"
 	"stash.appscode.dev/stash/apis/stash/v1beta1"
 
 	"github.com/appscode/go/crypto/rand"
 	. "github.com/onsi/gomega"
+	batchv1 "k8s.io/api/batch/v1"
 	core "k8s.io/api/core/v1"
 	kerr "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	meta_util "kmodules.xyz/client-go/meta"
 )
 
 func (fi *Invocation) GetRestoreSession(repoName string, transformFuncs ...func(restore *v1beta1.RestoreSession)) *v1beta1.RestoreSession {
@@ -88,3 +92,11 @@ func (f *Framework) EventuallyRestoreSessionPhase(meta metav1.ObjectMeta) Gomega
 		time.Second*7,
 	)
 }
+
+func (f *Framework) GetRestoreJob(restoreSessionName string) (*batchv1.Job, error) {
+	return f.KubeClient.BatchV1().Jobs(f.namespace).Get(getRestoreJobName(restoreSessionName), metav1.GetOptions{})
+}
+
+func getRestoreJobName(restoreSessionName string) string {
+	return meta_util.ValidNameWithPrefix(apis.PrefixStashRestore, strings.ReplaceAll(restoreSessionName, ".", "-"))
+}

test/e2e/framework/util.go

@@ -93,6 +93,11 @@ const (
 
 	WorkloadBackupBlueprint = "workload-backup-blueprint"
 	PvcBackupBlueprint      = "pvc-backup-blueprint"
+
+	TestFSGroup         = 2000
+	TestResourceRequest = "200Mi"
+	TestResourceLimit   = "300Mi"
+	TestUserID          = 2000
 )
 
 func (f *Framework) EventualEvent(meta metav1.ObjectMeta) GomegaAsyncAssertion {
@@ -1046,3 +1051,38 @@ func (f *Framework) EventuallyEventWritten(involvedObjectMeta metav1.ObjectMeta,
 		return false
 	}, time.Minute*2, time.Second*5)
 }
+
+func HasFSGroup(sc *core.PodSecurityContext) bool {
+	return sc != nil && sc.FSGroup != nil && *sc.FSGroup == TestFSGroup
+}
+
+func HasResources(containers []core.Container) bool {
+	for _, c := range containers {
+		if strings.HasPrefix(c.Name, "stash") ||
+			strings.HasPrefix(c.Name, "pvc-") ||
+			strings.HasPrefix(c.Name, "update-status") {
+			if c.Resources.Limits.Memory() != nil &&
+				c.Resources.Requests.Memory() != nil &&
+				c.Resources.Limits.Memory().String() == TestResourceLimit &&
+				c.Resources.Requests.Memory().String() == TestResourceRequest {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func HasSecurityContext(containers []core.Container) bool {
+	for _, c := range containers {
+		if strings.HasPrefix(c.Name, "stash") ||
+			strings.HasPrefix(c.Name, "pvc-") ||
+			strings.HasPrefix(c.Name, "update-status") {
+			if c.SecurityContext != nil &&
+				c.SecurityContext.RunAsUser != nil &&
+				*c.SecurityContext.RunAsUser == TestUserID {
+				return true
+			}
+		}
+	}
+	return false
+}