From 473acc4e2be8a15bf0a9f48270f1f4fae9fb036a 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 00000000..f2cd3918 --- /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 00000000..5f17e732 --- /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 5d911069..976e1efb 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 5a764008..20347648 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 886b60c7..f4d19220 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 db9081bc..b86ab5c2 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 7a780976..d34e9999 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 1a726752..cf247acb 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 56cd7f50..869c098f 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 349fe1c1..56ceb82e 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 0c9702c8..8de1f820 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 a333bd88..a1f68cea 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 7037e2ec..d54fa2a6 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 b09b2d54..ae0b0607 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 e1b48880..df513505 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 00000000..60314af4 --- /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 8cdaad11..45c74de3 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 fb7f6ac5..44322931 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