From abc0b9e9094d961901b7b593445b9c48c32f5b56 Mon Sep 17 00:00:00 2001 From: Ankit Chawla Date: Mon, 27 Jun 2022 16:52:28 +0530 Subject: [PATCH] Added support to set builder and fn pod specs via helm chart (#2461) The users can now set the pod spec for builder and fn pods via helm chart. Currently we have set some default securitycontext for the pods. Before there were no permissions set and the user would by default enter root when kubectl exec into pod. Now the permissions have been set and the user will not be able to access root directory in poolmgr and newdeploy pods. --- .../templates/buildermgr/configmap.yaml | 9 ++ .../templates/executor/configmap.yaml | 9 ++ charts/fission-all/values.yaml | 41 +++++++ go.mod | 2 +- pkg/apis/core/v1/const.go | 5 + pkg/buildermgr/buildermgr.go | 17 ++- pkg/buildermgr/envwatcher.go | 15 ++- pkg/executor/executor.go | 16 ++- .../executortype/newdeploy/newdeploy.go | 10 ++ .../executortype/newdeploy/newdeploymgr.go | 5 + pkg/executor/executortype/poolmgr/gp.go | 5 +- .../executortype/poolmgr/gp_deployment.go | 10 ++ pkg/executor/executortype/poolmgr/gpm.go | 6 +- .../poolmgr/poolpodcontroller_test.go | 2 +- pkg/executor/util/util.go | 15 +++ pkg/executor/util/util_test.go | 114 ++++++++++++++++++ pkg/utils/utils.go | 12 ++ skaffold.yaml | 2 + 18 files changed, 287 insertions(+), 8 deletions(-) create mode 100644 charts/fission-all/templates/buildermgr/configmap.yaml create mode 100644 charts/fission-all/templates/executor/configmap.yaml create mode 100644 pkg/executor/util/util_test.go diff --git a/charts/fission-all/templates/buildermgr/configmap.yaml b/charts/fission-all/templates/buildermgr/configmap.yaml new file mode 100644 index 000000000..f2cd39180 --- /dev/null +++ b/charts/fission-all/templates/buildermgr/configmap.yaml @@ -0,0 +1,9 @@ +{{- if .Values.builderPodSpec.enabled }} +apiVersion: v1 +kind: ConfigMap +metadata: + name: builder-podspec-patch +data: + spec: | + {{- toYaml .Values.builderPodSpec.podSpec | nindent 4 }} +{{- end -}} \ No newline at end of file diff --git a/charts/fission-all/templates/executor/configmap.yaml b/charts/fission-all/templates/executor/configmap.yaml new file mode 100644 index 000000000..5f17e732c --- /dev/null +++ b/charts/fission-all/templates/executor/configmap.yaml @@ -0,0 +1,9 @@ +{{- if .Values.runtimePodSpec.enabled }} +apiVersion: v1 +kind: ConfigMap +metadata: + name: runtime-podspec-patch +data: + spec: | + {{- toYaml .Values.runtimePodSpec.podSpec | nindent 4 }} +{{- end -}} \ No newline at end of file diff --git a/charts/fission-all/values.yaml b/charts/fission-all/values.yaml index 5d9110696..976e1efbb 100644 --- a/charts/fission-all/values.yaml +++ b/charts/fission-all/values.yaml @@ -741,3 +741,44 @@ mqt_keda: ## pprof: enabled: false + +## Enable runtimePodSpec and add spec to your poolmgr or newdeploy pods +## +runtimePodSpec: + + ## Setting it false by default so that integration tests pass + ## + enabled: false + + ## Checkout PodSpec in https://fission.io/docs/reference/crd-reference/#runtime + ## + podSpec: + + ## Default podspec to improve security of the pods + ## + securityContext: + fsGroup: 10001 + runAsGroup: 10001 + runAsNonRoot: true + runAsUser: 10001 + +## Enable builderPodSpec and add spec to your env builder pods +## +builderPodSpec: + + ## Setting it false by default so that integration tests pass + ## + enabled: false + + ## Checkout PodSpec in https://fission.io/docs/reference/crd-reference/#builder + ## + podSpec: + + ## Default podspec to improve security of the pods + ## + securityContext: + fsGroup: 10001 + runAsGroup: 10001 + runAsNonRoot: true + runAsUser: 10001 + diff --git a/go.mod b/go.mod index 5a7640087..203476486 100644 --- a/go.mod +++ b/go.mod @@ -55,6 +55,7 @@ require ( k8s.io/client-go v0.23.4 k8s.io/metrics v0.23.4 sigs.k8s.io/controller-runtime v0.10.2 + sigs.k8s.io/yaml v1.2.0 ) require ( @@ -181,5 +182,4 @@ require ( k8s.io/utils v0.0.0-20211116205334-6203023598ed // indirect sigs.k8s.io/json v0.0.0-20211020170558-c049b76a60c6 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.1 // indirect - sigs.k8s.io/yaml v1.2.0 // indirect ) diff --git a/pkg/apis/core/v1/const.go b/pkg/apis/core/v1/const.go index 886b60c75..f4d19220f 100644 --- a/pkg/apis/core/v1/const.go +++ b/pkg/apis/core/v1/const.go @@ -66,6 +66,11 @@ const ( StrategyTypeExecution = "execution" ) +const ( + RuntimePodSpecConfigmap = "runtime-podspec-patch" + BuilderPodSpecConfigmap = "builder-podspec-patch" +) + const ( SharedVolumeUserfunc = "userfunc" SharedVolumePackages = "packages" diff --git a/pkg/buildermgr/buildermgr.go b/pkg/buildermgr/buildermgr.go index db9081bca..b86ab5c28 100644 --- a/pkg/buildermgr/buildermgr.go +++ b/pkg/buildermgr/buildermgr.go @@ -22,11 +22,15 @@ import ( "github.com/pkg/errors" "go.uber.org/zap" + apiv1 "k8s.io/api/core/v1" k8sInformers "k8s.io/client-go/informers" + fv1 "github.com/fission/fission/pkg/apis/core/v1" "github.com/fission/fission/pkg/crd" + "github.com/fission/fission/pkg/executor/util" fetcherConfig "github.com/fission/fission/pkg/fetcher/config" genInformer "github.com/fission/fission/pkg/generated/informers/externalversions" + "github.com/fission/fission/pkg/utils" ) // Start the buildermgr service. @@ -48,7 +52,18 @@ func Start(ctx context.Context, logger *zap.Logger, storageSvcUrl string, envBui return errors.Wrap(err, "error making fetcher config") } - envWatcher := makeEnvironmentWatcher(bmLogger, fissionClient, kubernetesClient, fetcherConfig, envBuilderNamespace) + var podSpecPatch *apiv1.PodSpec + namespace, err := utils.GetCurrentNamespace() + if err != nil { + logger.Warn("Current namespace not found %v", zap.Error(err)) + } else { + podSpecPatch, err = util.GetSpecFromConfigMap(ctx, kubernetesClient, fv1.BuilderPodSpecConfigmap, namespace) + if err != nil { + logger.Warn("Either configmap is not found or error reading data %v", zap.Error(err)) + } + } + + envWatcher := makeEnvironmentWatcher(bmLogger, fissionClient, kubernetesClient, fetcherConfig, envBuilderNamespace, podSpecPatch) go envWatcher.watchEnvironments() k8sInformerFactory := k8sInformers.NewSharedInformerFactory(kubernetesClient, time.Minute*30) diff --git a/pkg/buildermgr/envwatcher.go b/pkg/buildermgr/envwatcher.go index 7a7809769..d34e99995 100644 --- a/pkg/buildermgr/envwatcher.go +++ b/pkg/buildermgr/envwatcher.go @@ -87,6 +87,7 @@ type ( fetcherConfig *fetcherConfig.Config builderImagePullPolicy apiv1.PullPolicy useIstio bool + podSpecPatch *apiv1.PodSpec } ) @@ -95,7 +96,8 @@ func makeEnvironmentWatcher( fissionClient versioned.Interface, kubernetesClient kubernetes.Interface, fetcherConfig *fetcherConfig.Config, - builderNamespace string) *environmentWatcher { + builderNamespace string, + podSpecPatch *apiv1.PodSpec) *environmentWatcher { useIstio := false enableIstio := os.Getenv("ENABLE_ISTIO") @@ -119,6 +121,7 @@ func makeEnvironmentWatcher( builderImagePullPolicy: builderImagePullPolicy, useIstio: useIstio, fetcherConfig: fetcherConfig, + podSpecPatch: podSpecPatch, } go envWatcher.service() @@ -503,6 +506,16 @@ func (envw *environmentWatcher) createBuilderDeployment(env *fv1.Environment, ns }, } + if envw.podSpecPatch != nil { + + updatedPodSpec, err := util.MergePodSpec(&pod.Spec, envw.podSpecPatch) + if err == nil { + pod.Spec = *updatedPodSpec + } else { + envw.logger.Warn("Failed to merge the specs: %v", zap.Error(err)) + } + } + pod.Spec = *(util.ApplyImagePullSecret(env.Spec.ImagePullSecret, pod.Spec)) deployment := &appsv1.Deployment{ diff --git a/pkg/executor/executor.go b/pkg/executor/executor.go index 1a726752a..cf247acb7 100644 --- a/pkg/executor/executor.go +++ b/pkg/executor/executor.go @@ -28,6 +28,7 @@ import ( "github.com/dchest/uniuri" "github.com/pkg/errors" "go.uber.org/zap" + apiv1 "k8s.io/api/core/v1" k8sInformers "k8s.io/client-go/informers" k8sCache "k8s.io/client-go/tools/cache" @@ -270,6 +271,17 @@ func StartExecutor(ctx context.Context, logger *zap.Logger, functionNamespace st executorInstanceID := strings.ToLower(uniuri.NewLen(8)) + var podSpecPatch *apiv1.PodSpec + namespace, err := utils.GetCurrentNamespace() + if err != nil { + logger.Warn("Current namespace not found %s", zap.Error(err)) + } else { + podSpecPatch, err = util.GetSpecFromConfigMap(ctx, kubernetesClient, fv1.RuntimePodSpecConfigmap, namespace) + if err != nil { + logger.Warn("Either configmap is not found or error reading data %v", zap.Error(err)) + } + } + logger.Info("Starting executor", zap.String("instanceID", executorInstanceID)) informerFactory := genInformer.NewSharedInformerFactory(fissionClient, time.Minute*30) @@ -288,7 +300,7 @@ func StartExecutor(ctx context.Context, logger *zap.Logger, functionNamespace st fissionClient, kubernetesClient, metricsClient, functionNamespace, fetcherConfig, executorInstanceID, funcInformer, pkgInformer, envInformer, - gpmPodInformer, gpmRsInformer) + gpmPodInformer, gpmRsInformer, podSpecPatch) if err != nil { return errors.Wrap(err, "pool manager creation failed") } @@ -304,7 +316,7 @@ func StartExecutor(ctx context.Context, logger *zap.Logger, functionNamespace st fissionClient, kubernetesClient, functionNamespace, fetcherConfig, executorInstanceID, funcInformer, envInformer, - ndmDeplInformer, ndmSvcInformer) + ndmDeplInformer, ndmSvcInformer, podSpecPatch) if err != nil { return errors.Wrap(err, "new deploy manager creation failed") } diff --git a/pkg/executor/executortype/newdeploy/newdeploy.go b/pkg/executor/executortype/newdeploy/newdeploy.go index 56cd7f50b..869c098fe 100644 --- a/pkg/executor/executortype/newdeploy/newdeploy.go +++ b/pkg/executor/executortype/newdeploy/newdeploy.go @@ -278,6 +278,16 @@ func (deploy *NewDeploy) getDeploymentSpec(ctx context.Context, fn *fv1.Function }, } + if deploy.podSpecPatch != nil { + + updatedPodSpec, err := util.MergePodSpec(&pod.Spec, deploy.podSpecPatch) + if err == nil { + pod.Spec = *updatedPodSpec + } else { + deploy.logger.Warn("Failed to merge the specs: %v", zap.Error(err)) + } + } + pod.Spec = *(util.ApplyImagePullSecret(env.Spec.ImagePullSecret, pod.Spec)) deployment := &appsv1.Deployment{ diff --git a/pkg/executor/executortype/newdeploy/newdeploymgr.go b/pkg/executor/executortype/newdeploy/newdeploymgr.go index 349fe1c1a..56ceb82eb 100644 --- a/pkg/executor/executortype/newdeploy/newdeploymgr.go +++ b/pkg/executor/executortype/newdeploy/newdeploymgr.go @@ -86,6 +86,8 @@ type ( svcListerSynced k8sCache.InformerSynced hpaops *hpautils.HpaOperations + + podSpecPatch *apiv1.PodSpec } ) @@ -101,6 +103,7 @@ func MakeNewDeploy( envInformer finformerv1.EnvironmentInformer, deplInformer appsinformers.DeploymentInformer, svcInformer coreinformers.ServiceInformer, + podSpecPatch *apiv1.PodSpec, ) (executortype.ExecutorType, error) { enableIstio := false if len(os.Getenv("ENABLE_ISTIO")) > 0 { @@ -129,6 +132,8 @@ func MakeNewDeploy( defaultIdlePodReapTime: 2 * time.Minute, hpaops: hpautils.NewHpaOperations(logger, kubernetesClient, instanceID), + + podSpecPatch: podSpecPatch, } nd.deplLister = deplInformer.Lister() diff --git a/pkg/executor/executortype/poolmgr/gp.go b/pkg/executor/executortype/poolmgr/gp.go index 0c9702c83..8de1f8200 100644 --- a/pkg/executor/executortype/poolmgr/gp.go +++ b/pkg/executor/executortype/poolmgr/gp.go @@ -78,6 +78,7 @@ type ( readyPodQueue workqueue.DelayingInterface poolInstanceID string // small random string to uniquify pod names instanceID string // poolmgr instance id + podSpecPatch *apiv1.PodSpec // TODO: move this field into fsCache podFSVCMap sync.Map } @@ -95,7 +96,8 @@ func MakeGenericPool( fsCache *fscache.FunctionServiceCache, fetcherConfig *fetcherConfig.Config, instanceID string, - enableIstio bool) *GenericPool { + enableIstio bool, + podSpecPatch *apiv1.PodSpec) *GenericPool { gpLogger := logger.Named("generic_pool") @@ -130,6 +132,7 @@ func MakeGenericPool( poolInstanceID: uniuri.NewLen(8), instanceID: instanceID, podFSVCMap: sync.Map{}, + podSpecPatch: podSpecPatch, } gp.runtimeImagePullPolicy = utils.GetImagePullPolicy(os.Getenv("RUNTIME_IMAGE_PULL_POLICY")) diff --git a/pkg/executor/executortype/poolmgr/gp_deployment.go b/pkg/executor/executortype/poolmgr/gp_deployment.go index a333bd887..a1f68cea7 100644 --- a/pkg/executor/executortype/poolmgr/gp_deployment.go +++ b/pkg/executor/executortype/poolmgr/gp_deployment.go @@ -150,6 +150,16 @@ func (gp *GenericPool) genDeploymentSpec(env *fv1.Environment) (*appsv1.Deployme }, } + if gp.podSpecPatch != nil { + + updatedPodSpec, err := util.MergePodSpec(&pod.Spec, gp.podSpecPatch) + if err == nil { + pod.Spec = *updatedPodSpec + } else { + gp.logger.Warn("Failed to merge the specs: %v", zap.Error(err)) + } + } + pod.Spec = *(util.ApplyImagePullSecret(env.Spec.ImagePullSecret, pod.Spec)) poolsize := getEnvPoolSize(env) diff --git a/pkg/executor/executortype/poolmgr/gpm.go b/pkg/executor/executortype/poolmgr/gpm.go index 7037e2ec7..d54fa2a61 100644 --- a/pkg/executor/executortype/poolmgr/gpm.go +++ b/pkg/executor/executortype/poolmgr/gpm.go @@ -92,6 +92,8 @@ type ( defaultIdlePodReapTime time.Duration poolPodC *PoolPodController + + podSpecPatch *apiv1.PodSpec } request struct { requestType @@ -119,6 +121,7 @@ func MakeGenericPoolManager( envInformer finformerv1.EnvironmentInformer, podInformer coreinformers.PodInformer, rsInformer appsinformers.ReplicaSetInformer, + podSpecPatch *apiv1.PodSpec, ) (executortype.ExecutorType, error) { gpmLogger := logger.Named("generic_pool_manager") @@ -150,6 +153,7 @@ func MakeGenericPoolManager( fetcherConfig: fetcherConfig, enableIstio: enableIstio, poolPodC: poolPodC, + podSpecPatch: podSpecPatch, } gpm.podLister = podInformer.Lister() gpm.podListerSynced = podInformer.Informer().HasSynced @@ -476,7 +480,7 @@ func (gpm *GenericPoolManager) service() { } pool = MakeGenericPool(gpm.logger, gpm.fissionClient, gpm.kubernetesClient, gpm.metricsClient, req.env, ns, gpm.namespace, gpm.fsCache, - gpm.fetcherConfig, gpm.instanceID, gpm.enableIstio) + gpm.fetcherConfig, gpm.instanceID, gpm.enableIstio, gpm.podSpecPatch) err = pool.setup(req.ctx) if err != nil { req.responseChannel <- &response{error: err} diff --git a/pkg/executor/executortype/poolmgr/poolpodcontroller_test.go b/pkg/executor/executortype/poolmgr/poolpodcontroller_test.go index b09b2d545..ae0b06072 100644 --- a/pkg/executor/executortype/poolmgr/poolpodcontroller_test.go +++ b/pkg/executor/executortype/poolmgr/poolpodcontroller_test.go @@ -78,7 +78,7 @@ func TestPoolPodControllerPodCleanup(t *testing.T) { fissionClient, kubernetesClient, metricsClient, fnNamespace, fetcherConfig, executorInstanceID, funcInformer, pkgInformer, envInformer, - gpmPodInformer, gpmRsInformer) + gpmPodInformer, gpmRsInformer, nil) if err != nil { t.Fatalf("Error creating generic pool manager: %v", err) } diff --git a/pkg/executor/util/util.go b/pkg/executor/util/util.go index e1b488800..df513505f 100644 --- a/pkg/executor/util/util.go +++ b/pkg/executor/util/util.go @@ -25,6 +25,7 @@ import ( apiv1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "sigs.k8s.io/yaml" fv1 "github.com/fission/fission/pkg/apis/core/v1" ) @@ -113,3 +114,17 @@ func ConvertConfigSecrets(ctx context.Context, fn *fv1.Function, kc kubernetes.I } return envFromSources, nil } + +func GetSpecFromConfigMap(ctx context.Context, kubeClient kubernetes.Interface, cm string, cmns string) (*apiv1.PodSpec, error) { + + podSpecPatch, err := kubeClient.CoreV1().ConfigMaps(cmns).Get(ctx, cm, metav1.GetOptions{}) + if err != nil { + return nil, err + } + + var additionalSpec apiv1.PodSpec + + err = yaml.Unmarshal([]byte(podSpecPatch.Data["spec"]), &additionalSpec) + + return &additionalSpec, err +} diff --git a/pkg/executor/util/util_test.go b/pkg/executor/util/util_test.go new file mode 100644 index 000000000..60314af49 --- /dev/null +++ b/pkg/executor/util/util_test.go @@ -0,0 +1,114 @@ +/* +Copyright 2016 The Fission Authors. + +Licensed 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 ( + "context" + "reflect" + "testing" + + apiv1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +func TestGetSpecFromConfigMap(t *testing.T) { + + kubeClient := fake.NewSimpleClientset() + + var permissionNum int64 = 10001 + var runAsNonRoot bool = true + + configMapData := make(map[string]string, 0) + specPatch := ` +securityContext: + fsGroup: 10001 + runAsGroup: 10001 + runAsNonRoot: true + runAsUser: 10001` + + configMapData["spec"] = specPatch + + testConfigMap := apiv1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-config-map", + Namespace: "fission", + }, + Data: configMapData, + } + + configmap, err := kubeClient.CoreV1().ConfigMaps("fission").Create(context.Background(), &testConfigMap, metav1.CreateOptions{}) + if err != nil { + t.Errorf("Error creating configmap %v", err) + } + + t.Logf("Configmap: %v", configmap.Data) + + testSpecPatch := apiv1.PodSpec{ + SecurityContext: &apiv1.PodSecurityContext{ + FSGroup: &permissionNum, + RunAsGroup: &permissionNum, + RunAsNonRoot: &runAsNonRoot, + RunAsUser: &permissionNum, + }, + } + tests := []struct { + name string + cm string + cmns string + want *apiv1.PodSpec + wantErr bool + }{ + { + name: "Configmap exists", + cm: "test-config-map", + cmns: "fission", + want: &testSpecPatch, + wantErr: false, + }, + { + name: "Configmap does not exists", + cm: "wrongname", + cmns: "fission", + want: nil, + wantErr: true, + }, + { + name: "Wrong namespace", + cm: "test-config-map", + cmns: "fissio", + want: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := GetSpecFromConfigMap(context.Background(), kubeClient, tt.cm, tt.cmns) + if (err != nil) != tt.wantErr { + t.Errorf("GetSpecFromConfigMap() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetSpecFromConfigMap() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 8cdaad115..45c74de3c 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -22,6 +22,7 @@ import ( "encoding/hex" "fmt" "io" + "io/ioutil" "net" "net/http" "os" @@ -214,3 +215,14 @@ func IsZip(filename string) (bool, error) { defer f.Close() return archiver.DefaultZip.Match(f) } + +// GetCurrentNamespace returns Kubernetes namespace of current Pod +func GetCurrentNamespace() (string, error) { + + // This file contains the namespace and can be found in each container. + body, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + if err != nil { + return "", err + } + return string(body), nil +} diff --git a/skaffold.yaml b/skaffold.yaml index fb7f6ac50..44322931a 100644 --- a/skaffold.yaml +++ b/skaffold.yaml @@ -47,6 +47,8 @@ deploy: influxdb.enabled: false storagesvc.archivePruner.enabled: true storagesvc.archivePruner.interval: "60" + runtimePodSpec.enabled: false + builderPodSpec.enabled: false repository: index.docker.io routerServiceType: LoadBalancer openTracing.enabled: false