From 985d94b5b834765fc9723c2f3b5aed2a80652079 Mon Sep 17 00:00:00 2001 From: Sanket Sudake Date: Thu, 8 Dec 2022 11:38:57 +0530 Subject: [PATCH] Consume podspec patch directly on executor/builder mounts (#2661) Signed-off-by: Sanket Sudake Signed-off-by: Sanket Sudake --- .../templates/buildermgr/configmap.yaml | 2 +- .../templates/buildermgr/deployment.yaml | 19 ++++- .../templates/executor/configmap.yaml | 2 +- .../templates/executor/deployment.yaml | 17 ++++- go.mod | 2 +- pkg/apis/core/v1/const.go | 4 +- pkg/buildermgr/buildermgr.go | 11 +-- pkg/executor/executor.go | 11 +-- .../newdeploy/newdeploymgr_test.go | 13 +--- pkg/executor/util/util.go | 17 ++--- pkg/executor/util/util_test.go | 75 ++++++++----------- pkg/utils/utils.go | 11 --- 12 files changed, 79 insertions(+), 105 deletions(-) diff --git a/charts/fission-all/templates/buildermgr/configmap.yaml b/charts/fission-all/templates/buildermgr/configmap.yaml index f2cd3918..506064f9 100644 --- a/charts/fission-all/templates/buildermgr/configmap.yaml +++ b/charts/fission-all/templates/buildermgr/configmap.yaml @@ -4,6 +4,6 @@ kind: ConfigMap metadata: name: builder-podspec-patch data: - spec: | + builder-podspec-patch.yaml: | {{- toYaml .Values.builderPodSpec.podSpec | nindent 4 }} {{- end -}} \ No newline at end of file diff --git a/charts/fission-all/templates/buildermgr/deployment.yaml b/charts/fission-all/templates/buildermgr/deployment.yaml index 274c7bb3..dd256095 100644 --- a/charts/fission-all/templates/buildermgr/deployment.yaml +++ b/charts/fission-all/templates/buildermgr/deployment.yaml @@ -44,7 +44,7 @@ spec: - name: FISSION_FUNCTION_NAMESPACE value: "{{ .Values.functionNamespace }}" - name: FISSION_DEFAULT_NAMESPACE - value: "{{ .Values.defaultNamespace }}" + value: "{{ .Values.defaultNamespace }}" - name: ENABLE_ISTIO value: "{{ .Values.enableIstio }}" - name: FETCHER_MINCPU @@ -61,8 +61,15 @@ spec: value: {{ .Values.pprof.enabled | quote }} - name: HELM_RELEASE_NAME value: {{ .Release.Name | quote }} - {{- include "fission-resource-namespace.envs" . | indent 8 }} + {{- include "fission-resource-namespace.envs" . | indent 8 }} {{- include "opentelemtry.envs" . | indent 8 }} + {{- if .Values.builderPodSpec.enabled }} + volumeMounts: + - name: builder-podspec-patch-volume + mountPath: /etc/fission/builder-podspec-patch.yaml + subPath: builder-podspec-patch.yaml + readOnly: true + {{- end }} ports: - containerPort: 8080 name: metrics @@ -75,11 +82,17 @@ spec: terminationMessagePolicy: {{ .Values.terminationMessagePolicy }} {{- end }} serviceAccountName: fission-buildermgr + {{- if .Values.builderPodSpec.enabled }} + volumes: + - name: builder-podspec-patch-volume + configMap: + name: builder-podspec-patch + {{- end }} {{- if .Values.priorityClassName }} priorityClassName: {{ .Values.priorityClassName }} {{- end }} {{- with .Values.imagePullSecrets }} - imagePullSecrets: + imagePullSecrets: {{- toYaml . | nindent 8 }} {{- end }} {{- if .Values.extraCoreComponentPodConfig }} diff --git a/charts/fission-all/templates/executor/configmap.yaml b/charts/fission-all/templates/executor/configmap.yaml index 5f17e732..de579377 100644 --- a/charts/fission-all/templates/executor/configmap.yaml +++ b/charts/fission-all/templates/executor/configmap.yaml @@ -4,6 +4,6 @@ kind: ConfigMap metadata: name: runtime-podspec-patch data: - spec: | + runtime-podspec-patch.yaml: | {{- toYaml .Values.runtimePodSpec.podSpec | nindent 4 }} {{- end -}} \ No newline at end of file diff --git a/charts/fission-all/templates/executor/deployment.yaml b/charts/fission-all/templates/executor/deployment.yaml index 14a11178..26cd0b53 100644 --- a/charts/fission-all/templates/executor/deployment.yaml +++ b/charts/fission-all/templates/executor/deployment.yaml @@ -42,7 +42,7 @@ spec: - name: FISSION_FUNCTION_NAMESPACE value: "{{ .Values.functionNamespace }}" - name: FISSION_DEFAULT_NAMESPACE - value: "{{ .Values.defaultNamespace }}" + value: "{{ .Values.defaultNamespace }}" - name: RUNTIME_IMAGE_PULL_POLICY value: "{{ .Values.pullPolicy }}" - name: ADOPT_EXISTING_RESOURCES @@ -96,6 +96,13 @@ spec: port: 8888 initialDelaySeconds: 35 periodSeconds: 5 + {{- if .Values.runtimePodSpec.enabled }} + volumeMounts: + - name: runtime-podspec-patch-volume + mountPath: /etc/fission/runtime-podspec-patch.yaml + subPath: runtime-podspec-patch.yaml + readOnly: true + {{- end }} ports: - containerPort: 8080 name: metrics @@ -116,13 +123,19 @@ spec: terminationMessagePolicy: {{ .Values.terminationMessagePolicy }} {{- end }} serviceAccountName: fission-executor + {{- if .Values.runtimePodSpec.enabled }} + volumes: + - name: runtime-podspec-patch-volume + configMap: + name: runtime-podspec-patch + {{- end }} {{- if .Values.executor.priorityClassName }} priorityClassName: {{ .Values.executor.priorityClassName }} {{- else if .Values.priorityClassName }} priorityClassName: {{ .Values.priorityClassName }} {{- end }} {{- with .Values.imagePullSecrets }} - imagePullSecrets: + imagePullSecrets: {{- toYaml . | nindent 8 }} {{- end }} {{- if .Values.extraCoreComponentPodConfig }} diff --git a/go.mod b/go.mod index 5983492a..a5040545 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/go-git/go-git/v5 v5.4.2 github.com/go-openapi/spec v0.20.7 github.com/golang-jwt/jwt/v4 v4.4.2 + github.com/google/go-cmp v0.5.9 github.com/gorilla/mux v1.8.0 github.com/graymeta/stow v0.2.8 github.com/hashicorp/go-multierror v1.1.1 @@ -108,7 +109,6 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/golang/snappy v0.0.4 // indirect github.com/google/gnostic v0.5.7-v3refs // indirect - github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.3.0 // indirect github.com/gotestyourself/gotestyourself v2.2.0+incompatible // indirect diff --git a/pkg/apis/core/v1/const.go b/pkg/apis/core/v1/const.go index 4544f59d..b74a47c4 100644 --- a/pkg/apis/core/v1/const.go +++ b/pkg/apis/core/v1/const.go @@ -67,8 +67,8 @@ const ( ) const ( - RuntimePodSpecConfigmap = "runtime-podspec-patch" - BuilderPodSpecConfigmap = "builder-podspec-patch" + RuntimePodSpecPath = "/etc/fission/runtime-podspec-patch.yaml" + BuilderPodSpecPath = "/etc/fission/builder-podspec-patch.yaml" ) const ( diff --git a/pkg/buildermgr/buildermgr.go b/pkg/buildermgr/buildermgr.go index 9e3b3311..9fc2feea 100644 --- a/pkg/buildermgr/buildermgr.go +++ b/pkg/buildermgr/buildermgr.go @@ -22,7 +22,6 @@ import ( "github.com/pkg/errors" "go.uber.org/zap" - apiv1 "k8s.io/api/core/v1" fv1 "github.com/fission/fission/pkg/apis/core/v1" "github.com/fission/fission/pkg/crd" @@ -50,15 +49,9 @@ func Start(ctx context.Context, logger *zap.Logger, storageSvcUrl string) error return errors.Wrap(err, "error making fetcher config") } - var podSpecPatch *apiv1.PodSpec - namespace, err := utils.GetCurrentNamespace() + podSpecPatch, err := util.GetSpecFromConfigMap(fv1.BuilderPodSpecPath) 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)) - } + logger.Warn("error reading data for pod spec patch", zap.String("path", fv1.BuilderPodSpecPath), zap.Error(err)) } envWatcher := makeEnvironmentWatcher(ctx, bmLogger, fissionClient, kubernetesClient, fetcherConfig, podSpecPatch) diff --git a/pkg/executor/executor.go b/pkg/executor/executor.go index 4b174ebb..25729881 100644 --- a/pkg/executor/executor.go +++ b/pkg/executor/executor.go @@ -28,7 +28,6 @@ import ( "github.com/dchest/uniuri" "github.com/pkg/errors" "go.uber.org/zap" - apiv1 "k8s.io/api/core/v1" k8sCache "k8s.io/client-go/tools/cache" fv1 "github.com/fission/fission/pkg/apis/core/v1" @@ -271,15 +270,9 @@ func StartExecutor(ctx context.Context, logger *zap.Logger, port int) error { executorInstanceID := strings.ToLower(uniuri.NewLen(8)) - var podSpecPatch *apiv1.PodSpec - namespace, err := utils.GetCurrentNamespace() + podSpecPatch, err := util.GetSpecFromConfigMap(fv1.RuntimePodSpecPath) 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.Warn("error reading data for pod spec patch", zap.String("path", fv1.RuntimePodSpecPath), zap.Error(err)) } logger.Info("Starting executor", zap.String("instanceID", executorInstanceID)) diff --git a/pkg/executor/executortype/newdeploy/newdeploymgr_test.go b/pkg/executor/executortype/newdeploy/newdeploymgr_test.go index 39ddefed..4ebf65de 100644 --- a/pkg/executor/executortype/newdeploy/newdeploymgr_test.go +++ b/pkg/executor/executortype/newdeploy/newdeploymgr_test.go @@ -18,7 +18,6 @@ import ( k8sCache "k8s.io/client-go/tools/cache" fv1 "github.com/fission/fission/pkg/apis/core/v1" - "github.com/fission/fission/pkg/executor/util" fetcherConfig "github.com/fission/fission/pkg/fetcher/config" fClient "github.com/fission/fission/pkg/generated/clientset/versioned/fake" genInformer "github.com/fission/fission/pkg/generated/informers/externalversions" @@ -65,23 +64,13 @@ func TestRefreshFuncPods(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err = BuildConfigMap(ctx, kubernetesClient, functionNamespace, fv1.RuntimePodSpecConfigmap, map[string]string{}) - if err != nil { - t.Fatalf("Error building configmap: %s", err) - } - - podSpecPatch, err := util.GetSpecFromConfigMap(ctx, kubernetesClient, fv1.RuntimePodSpecConfigmap, functionNamespace) - if err != nil { - t.Fatalf("Error creating pod spec: %s", err) - } - fetcherConfig, err := fetcherConfig.MakeFetcherConfig("/userfunc") if err != nil { t.Fatalf("Error creating fetcher config: %s", err) } executor, err := MakeNewDeploy(ctx, logger, fissionClient, kubernetesClient, fetcherConfig, "test", - funcInformer, envInformer, ndmInformerFactory, podSpecPatch) + funcInformer, envInformer, ndmInformerFactory, nil) if err != nil { t.Fatalf("new deploy manager creation failed: %s", err) } diff --git a/pkg/executor/util/util.go b/pkg/executor/util/util.go index 05840348..acd7fa90 100644 --- a/pkg/executor/util/util.go +++ b/pkg/executor/util/util.go @@ -20,6 +20,7 @@ import ( "context" "errors" "fmt" + "os" "strings" "sync" "time" @@ -119,18 +120,14 @@ 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{}) +func GetSpecFromConfigMap(filePath string) (*apiv1.PodSpec, error) { + content, err := os.ReadFile(filePath) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading YAML file %s: %w", filePath, err) } - - var additionalSpec apiv1.PodSpec - - err = yaml.Unmarshal([]byte(podSpecPatch.Data["spec"]), &additionalSpec) - - return &additionalSpec, err + additionalSpec := &apiv1.PodSpec{} + err = yaml.UnmarshalStrict(content, &additionalSpec) + return additionalSpec, err } func GetObjectReaperInterval(logger *zap.Logger, executorType fv1.ExecutorType, defaultReaperInterval uint) uint { diff --git a/pkg/executor/util/util_test.go b/pkg/executor/util/util_test.go index cdc25f73..7e148dcc 100644 --- a/pkg/executor/util/util_test.go +++ b/pkg/executor/util/util_test.go @@ -17,57 +17,45 @@ limitations under the License. package util import ( - "context" "fmt" "os" "reflect" "testing" - apiv1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/fake" + "github.com/google/go-cmp/cmp" fv1 "github.com/fission/fission/pkg/apis/core/v1" "github.com/fission/fission/pkg/utils/loggerfactory" + apiv1 "k8s.io/api/core/v1" ) func TestGetSpecFromConfigMap(t *testing.T) { - - kubeClient := fake.NewSimpleClientset() - - var permissionNum int64 = 10001 - var runAsNonRoot bool = true - - configMapData := make(map[string]string, 0) + runtimePodSpecPath := "runtime-podspec-patch.yaml" + tempDir := t.TempDir() 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, - } - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - configmap, err := kubeClient.CoreV1().ConfigMaps("fission").Create(ctx, &testConfigMap, metav1.CreateOptions{}) + err := os.WriteFile(tempDir+"/"+runtimePodSpecPath, []byte(specPatch), 0644) if err != nil { - t.Errorf("Error creating configmap %v", err) + t.Errorf("Error writing to file %s", err) + } + specPatch2 := ` +securityContext2: +securityContext: + fsGroup: "invalida_input" + runAsGroup: 10001 + runAsNonRoot: true + runAsUser: 10001` + err = os.WriteFile(tempDir+"/"+runtimePodSpecPath+"2", []byte(specPatch2), 0644) + if err != nil { + t.Errorf("Error writing to file %s", err) } - t.Logf("Configmap: %v", configmap.Data) + var permissionNum int64 = 10001 + var runAsNonRoot bool = true testSpecPatch := apiv1.PodSpec{ SecurityContext: &apiv1.PodSecurityContext{ @@ -79,42 +67,41 @@ securityContext: } tests := []struct { name string - cm string - cmns string + path string want *apiv1.PodSpec wantErr bool }{ { - name: "Configmap exists", - cm: "test-config-map", - cmns: "fission", + name: "File exists with valid data", + path: tempDir + "/" + runtimePodSpecPath, want: &testSpecPatch, wantErr: false, }, { - name: "Configmap does not exists", - cm: "wrongname", - cmns: "fission", + name: "File with invalid data", + path: tempDir + "/" + runtimePodSpecPath + "2", want: nil, wantErr: true, }, { - name: "Wrong namespace", - cm: "test-config-map", - cmns: "fissio", + name: "File does not exist", + path: tempDir + "/" + "notexist", want: nil, wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetSpecFromConfigMap(ctx, kubeClient, tt.cm, tt.cmns) + got, err := GetSpecFromConfigMap(tt.path) if (err != nil) != tt.wantErr { t.Errorf("GetSpecFromConfigMap() error = %v, wantErr %v", err, tt.wantErr) return } + if err != nil { + return + } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetSpecFromConfigMap() got = %v, want %v", got, tt.want) + t.Errorf("GetSpecFromConfigMap() diff = %s", cmp.Diff(tt.want, got)) } }) } diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 67705cae..e0bbf34b 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -216,17 +216,6 @@ func IsZip(filename string) (bool, error) { 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 := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") - if err != nil { - return "", err - } - return string(body), nil -} - func GetStringValueFromEnv(envVar string) (string, error) { v := os.Getenv(envVar) if v == "" {