Merge pull request #86 from cdapio/feature/CDAP-18995-Env

[CDAP-18995] Add env var support across all CDAP services

api/v1alpha1/cdapmaster_types.go

@@ -42,6 +42,8 @@ type CDAPMasterSpec struct {
 	SecuritySecret string `json:"securitySecret,omitempty"`
 	// ServiceAccountName is the service account for all the service pods.
 	ServiceAccountName string `json:"serviceAccountName,omitempty"`
+	// Env is a list of environment variables for the all service containers.
+	Env []corev1.EnvVar `json:"env,omitempty"`
 	// LocationURI is an URI specifying an object storage for CDAP.
 	LocationURI string `json:"locationURI"`
 	// Config is a set of configurations that goes into cdap-site.xml.

config/crd/bases/cdap.cdap.io_cdapmasters.yaml

@@ -6292,6 +6292,106 @@ spec:
                 mount path. This adds ConfigMap data to the directory specified by
                 the volume mount path.
               type: object
+            env:
+              description: Env is a list of environment variables for the all service
+                containers.
+              items:
+                description: EnvVar represents an environment variable present in
+                  a Container.
+                properties:
+                  name:
+                    description: Name of the environment variable. Must be a C_IDENTIFIER.
+                    type: string
+                  value:
+                    description: 'Variable references $(VAR_NAME) are expanded using
+                      the previous defined environment variables in the container
+                      and any service environment variables. If a variable cannot
+                      be resolved, the reference in the input string will be unchanged.
+                      The $(VAR_NAME) syntax can be escaped with a double $$, ie:
+                      $$(VAR_NAME). Escaped references will never be expanded, regardless
+                      of whether the variable exists or not. Defaults to "".'
+                    type: string
+                  valueFrom:
+                    description: Source for the environment variable's value. Cannot
+                      be used if value is not empty.
+                    properties:
+                      configMapKeyRef:
+                        description: Selects a key of a ConfigMap.
+                        properties:
+                          key:
+                            description: The key to select.
+                            type: string
+                          name:
+                            description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+                              TODO: Add other useful fields. apiVersion, kind, uid?'
+                            type: string
+                          optional:
+                            description: Specify whether the ConfigMap or its key
+                              must be defined
+                            type: boolean
+                        required:
+                        - key
+                        type: object
+                      fieldRef:
+                        description: 'Selects a field of the pod: supports metadata.name,
+                          metadata.namespace, metadata.labels, metadata.annotations,
+                          spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP,
+                          status.podIPs.'
+                        properties:
+                          apiVersion:
+                            description: Version of the schema the FieldPath is written
+                              in terms of, defaults to "v1".
+                            type: string
+                          fieldPath:
+                            description: Path of the field to select in the specified
+                              API version.
+                            type: string
+                        required:
+                        - fieldPath
+                        type: object
+                      resourceFieldRef:
+                        description: 'Selects a resource of the container: only resources
+                          limits and requests (limits.cpu, limits.memory, limits.ephemeral-storage,
+                          requests.cpu, requests.memory and requests.ephemeral-storage)
+                          are currently supported.'
+                        properties:
+                          containerName:
+                            description: 'Container name: required for volumes, optional
+                              for env vars'
+                            type: string
+                          divisor:
+                            description: Specifies the output format of the exposed
+                              resources, defaults to "1"
+                            type: string
+                          resource:
+                            description: 'Required: resource to select'
+                            type: string
+                        required:
+                        - resource
+                        type: object
+                      secretKeyRef:
+                        description: Selects a key of a secret in the pod's namespace
+                        properties:
+                          key:
+                            description: The key of the secret to select from.  Must
+                              be a valid secret key.
+                            type: string
+                          name:
+                            description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+                              TODO: Add other useful fields. apiVersion, kind, uid?'
+                            type: string
+                          optional:
+                            description: Specify whether the Secret or its key must
+                              be defined
+                            type: boolean
+                        required:
+                        - key
+                        type: object
+                    type: object
+                required:
+                - name
+                type: object
+              type: array
             image:
               description: Image is the docker image name for the CDAP backend.
               type: string

controllers/deployment.go

@@ -3,6 +3,7 @@ package controllers
 import (
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 
 	"cdap.io/cdap-operator/api/v1alpha1"
@@ -186,7 +187,10 @@ func buildStatefulSets(master *v1alpha1.CDAPMaster, name string, services Servic
 			continue
 		}
 
-		c := serviceContainerSpec(ss, master, dataDir, s)
+		c, err := serviceContainerSpec(ss, master, dataDir, s)
+		if err != nil {
+			return nil, err
+		}
 		spec = spec.withContainer(c)
 		if err := addSystemMetricsServiceIfEnabled(spec, master, ss, dataDir, c); err != nil {
 			return nil, err
@@ -246,7 +250,10 @@ func addSystemMetricsServiceIfEnabled(stsSpec *StatefulSpec, master *v1alpha1.CD
 	if err != nil {
 		return err
 	}
-	c := serviceContainerSpec(ss, master, dataDir, serviceSystemMetricsExporter)
+	c, err := serviceContainerSpec(ss, master, dataDir, serviceSystemMetricsExporter)
+	if err != nil {
+		return err
+	}
 	stsSpec = stsSpec.withContainer(c)
 	// add env variable to start jmx server in the main container
 	varAdded := false
@@ -315,7 +322,11 @@ func buildDeployment(master *v1alpha1.CDAPMaster, name string, services ServiceG
 		if ss == nil {
 			continue
 		}
-		c := serviceContainerSpec(ss, master, dataDir, s)
+
+		c, err := serviceContainerSpec(ss, master, dataDir, s)
+		if err != nil {
+			return nil, err
+		}
 		spec = spec.withContainer(c)
 
 		// Adding a label to allow k8s service selector to easily find the pod
@@ -343,14 +354,56 @@ func buildDeployment(master *v1alpha1.CDAPMaster, name string, services ServiceG
 	return spec, nil
 }
 
+type ByEnvKey []corev1.EnvVar
+
+func (k ByEnvKey) Len() int           { return len(k) }
+func (k ByEnvKey) Swap(i, j int)      { k[i], k[j] = k[j], k[i] }
+func (k ByEnvKey) Less(i, j int) bool { return k[i].Name < k[j].Name }
+
+func mergeEnvVars(baseEnvVars []corev1.EnvVar, overwriteEnvVars []corev1.EnvVar) ([]corev1.EnvVar, error) {
+	// Merge base and overwrite environment variables
+	envMap := make(map[string]corev1.EnvVar)
+	// Add base environment variables.
+	for _, baseEnvVar := range baseEnvVars {
+		if _, ok := envMap[baseEnvVar.Name]; ok {
+			return nil, fmt.Errorf("duplicate env var %q in base slice", baseEnvVar.Name)
+		}
+		envMap[baseEnvVar.Name] = baseEnvVar
+	}
+
+	// Add and overwrite the provided environment variables.
+	// Maintain a seen map and throw an error if there are duplicates in the overwrite env var slice.
+	seenVars := make(map[string]bool)
+	for _, envVar := range overwriteEnvVars {
+		if _, ok := seenVars[envVar.Name]; ok {
+			return nil, fmt.Errorf("duplicate env var %q in overwrite slice", envVar.Name)
+		}
+		seenVars[envVar.Name] = true
+		envMap[envVar.Name] = envVar
+	}
+
+	// Convert the map to a sorted slice.
+	env := []corev1.EnvVar{}
+	for _, envVar := range envMap {
+		env = append(env, envVar)
+	}
+	sort.Sort(ByEnvKey(env))
+	return env, nil
+}
+
 func serviceContainerSpec(ss *v1alpha1.CDAPServiceSpec,
-	master *v1alpha1.CDAPMaster, dataDir string, service ServiceName) *ContainerSpec {
-	env := addJavaMaxHeapEnvIfNotPresent(ss.Env, ss.Resources)
+	master *v1alpha1.CDAPMaster, dataDir string, service ServiceName) (*ContainerSpec, error) {
+	// Merge environment variables between service spec and master spec
+	env, err := mergeEnvVars(master.Spec.Env, ss.Env)
+	if err != nil {
+		return nil, fmt.Errorf("failed to merge env vars for service %q with error: %v", service, err)
+	}
+	env = addJavaMaxHeapEnvIfNotPresent(env, ss.Resources)
 	c := newContainerSpec(master, service, dataDir).setResources(ss.Resources).setEnv(env).setLifecycle(ss.Lifecycle)
 	if service == serviceUserInterface {
 		c = updateSpecForUserInterface(master, c)
 	}
-	return c
+	return c, nil
 }
 
 // Return a list of reconciler objects (e.g. statefulsets, deployment, NodePort service) for the given deployment plan

controllers/deployment_test.go

@@ -4,8 +4,11 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"testing"
 
 	"cdap.io/cdap-operator/api/v1alpha1"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/nsf/jsondiff"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -254,3 +257,103 @@ var _ = Describe("Controller Suite", func() {
 		})
 	})
 })
+
+func TestMergeEnvVars(t *testing.T) {
+	testCases := []struct {
+		description      string
+		baseEnvVars      []corev1.EnvVar
+		overwriteEnvVars []corev1.EnvVar
+		wantEnv          []corev1.EnvVar
+		wantErr          error
+	}{
+		{
+			description:      "Empty slices returns no env vars",
+			baseEnvVars:      []corev1.EnvVar{},
+			overwriteEnvVars: []corev1.EnvVar{},
+			wantEnv:          []corev1.EnvVar{},
+		},
+		{
+			description:      "Only one env var in base slice returns one env var",
+			baseEnvVars:      []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
+			overwriteEnvVars: []corev1.EnvVar{},
+			wantEnv:          []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
+		},
+		{
+			description:      "Only one env var in overwrite slice returns one env var",
+			baseEnvVars:      []corev1.EnvVar{},
+			overwriteEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
+			wantEnv:          []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
+		},
+		{
+			description:      "One different env var in each slice returns two env var",
+			baseEnvVars:      []corev1.EnvVar{corev1.EnvVar{Name: "test-a", Value: "test-value-a"}},
+			overwriteEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test-b", Value: "test-value-b"}},
+			wantEnv: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "test-b", Value: "test-value-b"},
+			},
+		},
+		{
+			description: "Env var in overwrite slice overwrites expected env var from base slice",
+			baseEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "test-b", Value: "test-value-b"},
+			},
+			overwriteEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-b", Value: "test-value-d"},
+				corev1.EnvVar{Name: "test-c", Value: "test-value-c"},
+			},
+			wantEnv: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "test-b", Value: "test-value-d"},
+				corev1.EnvVar{Name: "test-c", Value: "test-value-c"},
+			},
+		},
+		{
+			description: "Multiple env vars in both slices returns env vars in sorted order",
+			baseEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "c", Value: "test-value-c"},
+			},
+			overwriteEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "d", Value: "test-value-d"},
+				corev1.EnvVar{Name: "b", Value: "test-value-b"},
+			},
+			wantEnv: []corev1.EnvVar{
+				corev1.EnvVar{Name: "a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "b", Value: "test-value-b"},
+				corev1.EnvVar{Name: "c", Value: "test-value-c"},
+				corev1.EnvVar{Name: "d", Value: "test-value-d"},
+			},
+		},
+		{
+			description: "Duplicate env var keys in base slice returns error",
+			baseEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "test-a", Value: "test-value-b"},
+			},
+			overwriteEnvVars: []corev1.EnvVar{},
+			wantErr:          cmpopts.AnyError,
+		},
+		{
+			description: "Duplicate env var keys in overwrite slice returns error",
+			baseEnvVars: []corev1.EnvVar{},
+			overwriteEnvVars: []corev1.EnvVar{
+				corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
+				corev1.EnvVar{Name: "test-a", Value: "test-value-b"},
+			},
+			wantErr: cmpopts.AnyError,
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			gotEnv, err := mergeEnvVars(testCase.baseEnvVars, testCase.overwriteEnvVars)
+			if got, want := gotEnv, testCase.wantEnv; !cmp.Equal(got, want) {
+				t.Errorf("mergeEnvVars(%+v, %+v): unexpected env slice: got %+v, want %+v", testCase.baseEnvVars, testCase.overwriteEnvVars, got, want)
+			}
+			if got, want := err, testCase.wantErr; !cmp.Equal(got, want, cmpopts.EquateErrors()) {
+				t.Errorf("mergeEnvVars(%+v, %+v): unexpected env slice: got %v, want %v", testCase.baseEnvVars, testCase.overwriteEnvVars, got, want)
+			}
+		})
+	}
+}

controllers/testdata/appfabric.json

@@ -73,6 +73,14 @@
               "--env=k8s"
             ],
             "env": [
+              {
+                "name": "all-services-test",
+                "value": "some-value-overridden"
+              },
+              {
+                "name": "appfabric-env-var-test",
+                "value": "some-value"
+              },
               {
                 "name": "JAVA_HEAPMAX",
                 "value": "-Xmx62914560"

controllers/testdata/artifactcache.json

@@ -208,6 +208,10 @@
               "--env=k8s"
             ],
             "env":[
+              {
+                "name": "all-services-test",
+                "value": "some-value"
+              },
               {
                 "name":"JAVA_HEAPMAX",
                 "value":"-Xmx125829120"

controllers/testdata/authentication.json

@@ -73,6 +73,10 @@
               "--env=k8s"
             ],
             "env": [
+              {
+                "name": "all-services-test",
+                "value": "some-value"
+              },
               {
                 "name": "JAVA_HEAPMAX",
                 "value": "-Xmx62914560"