Skip to content

Commit 81e90b2

Browse files
dli357dli357
committed
[CDAP-18995] Add env var support across all CDAP services
[CDAP-18995] Switched to using maps and added unit test
1 parent bc3e48f commit 81e90b2

21 files changed

+354
-9
lines changed

‎api/v1alpha1/cdapmaster_types.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ type CDAPMasterSpec struct {
4242
SecuritySecret string `json:"securitySecret,omitempty"`
4343
// ServiceAccountName is the service account for all the service pods.
4444
ServiceAccountName string `json:"serviceAccountName,omitempty"`
45+
// Env is a list of environment variables for the all service containers.
46+
Env []corev1.EnvVar `json:"env,omitempty"`
4547
// LocationURI is an URI specifying an object storage for CDAP.
4648
LocationURI string `json:"locationURI"`
4749
// Config is a set of configurations that goes into cdap-site.xml.

‎api/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 8 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6292,6 +6292,106 @@ spec:
62926292
mount path. This adds ConfigMap data to the directory specified by
62936293
the volume mount path.
62946294
type: object
6295+
env:
6296+
description: Env is a list of environment variables for the all service
6297+
containers.
6298+
items:
6299+
description: EnvVar represents an environment variable present in
6300+
a Container.
6301+
properties:
6302+
name:
6303+
description: Name of the environment variable. Must be a C_IDENTIFIER.
6304+
type: string
6305+
value:
6306+
description: 'Variable references $(VAR_NAME) are expanded using
6307+
the previous defined environment variables in the container
6308+
and any service environment variables. If a variable cannot
6309+
be resolved, the reference in the input string will be unchanged.
6310+
The $(VAR_NAME) syntax can be escaped with a double $$, ie:
6311+
$$(VAR_NAME). Escaped references will never be expanded, regardless
6312+
of whether the variable exists or not. Defaults to "".'
6313+
type: string
6314+
valueFrom:
6315+
description: Source for the environment variable's value. Cannot
6316+
be used if value is not empty.
6317+
properties:
6318+
configMapKeyRef:
6319+
description: Selects a key of a ConfigMap.
6320+
properties:
6321+
key:
6322+
description: The key to select.
6323+
type: string
6324+
name:
6325+
description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
6326+
TODO: Add other useful fields. apiVersion, kind, uid?'
6327+
type: string
6328+
optional:
6329+
description: Specify whether the ConfigMap or its key
6330+
must be defined
6331+
type: boolean
6332+
required:
6333+
- key
6334+
type: object
6335+
fieldRef:
6336+
description: 'Selects a field of the pod: supports metadata.name,
6337+
metadata.namespace, metadata.labels, metadata.annotations,
6338+
spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP,
6339+
status.podIPs.'
6340+
properties:
6341+
apiVersion:
6342+
description: Version of the schema the FieldPath is written
6343+
in terms of, defaults to "v1".
6344+
type: string
6345+
fieldPath:
6346+
description: Path of the field to select in the specified
6347+
API version.
6348+
type: string
6349+
required:
6350+
- fieldPath
6351+
type: object
6352+
resourceFieldRef:
6353+
description: 'Selects a resource of the container: only resources
6354+
limits and requests (limits.cpu, limits.memory, limits.ephemeral-storage,
6355+
requests.cpu, requests.memory and requests.ephemeral-storage)
6356+
are currently supported.'
6357+
properties:
6358+
containerName:
6359+
description: 'Container name: required for volumes, optional
6360+
for env vars'
6361+
type: string
6362+
divisor:
6363+
description: Specifies the output format of the exposed
6364+
resources, defaults to "1"
6365+
type: string
6366+
resource:
6367+
description: 'Required: resource to select'
6368+
type: string
6369+
required:
6370+
- resource
6371+
type: object
6372+
secretKeyRef:
6373+
description: Selects a key of a secret in the pod's namespace
6374+
properties:
6375+
key:
6376+
description: The key of the secret to select from. Must
6377+
be a valid secret key.
6378+
type: string
6379+
name:
6380+
description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
6381+
TODO: Add other useful fields. apiVersion, kind, uid?'
6382+
type: string
6383+
optional:
6384+
description: Specify whether the Secret or its key must
6385+
be defined
6386+
type: boolean
6387+
required:
6388+
- key
6389+
type: object
6390+
type: object
6391+
required:
6392+
- name
6393+
type: object
6394+
type: array
62956395
image:
62966396
description: Image is the docker image name for the CDAP backend.
62976397
type: string

‎controllers/deployment.go‎

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package controllers
33
import (
44
"fmt"
55
"reflect"
6+
"sort"
67
"strings"
78

89
"cdap.io/cdap-operator/api/v1alpha1"
@@ -186,7 +187,10 @@ func buildStatefulSets(master *v1alpha1.CDAPMaster, name string, services Servic
186187
continue
187188
}
188189

189-
c := serviceContainerSpec(ss, master, dataDir, s)
190+
c, err := serviceContainerSpec(ss, master, dataDir, s)
191+
if err != nil {
192+
return nil, err
193+
}
190194
spec = spec.withContainer(c)
191195
if err := addSystemMetricsServiceIfEnabled(spec, master, ss, dataDir, c); err != nil {
192196
return nil, err
@@ -246,7 +250,10 @@ func addSystemMetricsServiceIfEnabled(stsSpec *StatefulSpec, master *v1alpha1.CD
246250
if err != nil {
247251
return err
248252
}
249-
c := serviceContainerSpec(ss, master, dataDir, serviceSystemMetricsExporter)
253+
c, err := serviceContainerSpec(ss, master, dataDir, serviceSystemMetricsExporter)
254+
if err != nil {
255+
return err
256+
}
250257
stsSpec = stsSpec.withContainer(c)
251258
// add env variable to start jmx server in the main container
252259
varAdded := false
@@ -315,7 +322,11 @@ func buildDeployment(master *v1alpha1.CDAPMaster, name string, services ServiceG
315322
if ss == nil {
316323
continue
317324
}
318-
c := serviceContainerSpec(ss, master, dataDir, s)
325+
326+
c, err := serviceContainerSpec(ss, master, dataDir, s)
327+
if err != nil {
328+
return nil, err
329+
}
319330
spec = spec.withContainer(c)
320331

321332
// 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
343354
return spec, nil
344355
}
345356

357+
type ByEnvKey []corev1.EnvVar
358+
359+
func (k ByEnvKey) Len() int { return len(k) }
360+
func (k ByEnvKey) Swap(i, j int) { k[i], k[j] = k[j], k[i] }
361+
func (k ByEnvKey) Less(i, j int) bool { return k[i].Name < k[j].Name }
362+
363+
func mergeEnvVars(baseEnvVars []corev1.EnvVar, overwriteEnvVars []corev1.EnvVar) ([]corev1.EnvVar, error) {
364+
// Merge base and overwrite environment variables
365+
envMap := make(map[string]corev1.EnvVar)
366+
// Add base environment variables.
367+
for _, baseEnvVar := range baseEnvVars {
368+
if _, ok := envMap[baseEnvVar.Name]; ok {
369+
return nil, fmt.Errorf("duplicate env var %q in base slice", baseEnvVar.Name)
370+
}
371+
envMap[baseEnvVar.Name] = baseEnvVar
372+
}
373+
374+
// Add and overwrite the provided environment variables.
375+
// Maintain a seen map and throw an error if there are duplicates in the overwrite env var slice.
376+
seenVars := make(map[string]bool)
377+
for _, envVar := range overwriteEnvVars {
378+
if _, ok := seenVars[envVar.Name]; ok {
379+
return nil, fmt.Errorf("duplicate env var %q in overwrite slice", envVar.Name)
380+
}
381+
seenVars[envVar.Name] = true
382+
envMap[envVar.Name] = envVar
383+
}
384+
385+
// Convert the map to a sorted slice.
386+
env := []corev1.EnvVar{}
387+
for _, envVar := range envMap {
388+
env = append(env, envVar)
389+
}
390+
sort.Sort(ByEnvKey(env))
391+
return env, nil
392+
}
393+
346394
func serviceContainerSpec(ss *v1alpha1.CDAPServiceSpec,
347-
master *v1alpha1.CDAPMaster, dataDir string, service ServiceName) *ContainerSpec {
348-
env := addJavaMaxHeapEnvIfNotPresent(ss.Env, ss.Resources)
395+
master *v1alpha1.CDAPMaster, dataDir string, service ServiceName) (*ContainerSpec, error) {
396+
// Merge environment variables between service spec and master spec
397+
env, err := mergeEnvVars(master.Spec.Env, ss.Env)
398+
if err != nil {
399+
return nil, fmt.Errorf("failed to merge env vars for service %q with error: %v", service, err)
400+
}
401+
env = addJavaMaxHeapEnvIfNotPresent(env, ss.Resources)
349402
c := newContainerSpec(master, service, dataDir).setResources(ss.Resources).setEnv(env).setLifecycle(ss.Lifecycle)
350403
if service == serviceUserInterface {
351404
c = updateSpecForUserInterface(master, c)
352405
}
353-
return c
406+
return c, nil
354407
}
355408

356409
// Return a list of reconciler objects (e.g. statefulsets, deployment, NodePort service) for the given deployment plan

‎controllers/deployment_test.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@ import (
44
"encoding/json"
55
"fmt"
66
"io/ioutil"
7+
"testing"
78

89
"cdap.io/cdap-operator/api/v1alpha1"
10+
"github.com/google/go-cmp/cmp"
11+
"github.com/google/go-cmp/cmp/cmpopts"
912
"github.com/nsf/jsondiff"
1013
. "github.com/onsi/ginkgo"
1114
. "github.com/onsi/gomega"
@@ -254,3 +257,103 @@ var _ = Describe("Controller Suite", func() {
254257
})
255258
})
256259
})
260+
261+
func TestMergeEnvVars(t *testing.T) {
262+
testCases := []struct {
263+
description string
264+
baseEnvVars []corev1.EnvVar
265+
overwriteEnvVars []corev1.EnvVar
266+
wantEnv []corev1.EnvVar
267+
wantErr error
268+
}{
269+
{
270+
description: "Empty slices returns no env vars",
271+
baseEnvVars: []corev1.EnvVar{},
272+
overwriteEnvVars: []corev1.EnvVar{},
273+
wantEnv: []corev1.EnvVar{},
274+
},
275+
{
276+
description: "Only one env var in base slice returns one env var",
277+
baseEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
278+
overwriteEnvVars: []corev1.EnvVar{},
279+
wantEnv: []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
280+
},
281+
{
282+
description: "Only one env var in overwrite slice returns one env var",
283+
baseEnvVars: []corev1.EnvVar{},
284+
overwriteEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
285+
wantEnv: []corev1.EnvVar{corev1.EnvVar{Name: "test", Value: "test-value"}},
286+
},
287+
{
288+
description: "One different env var in each slice returns two env var",
289+
baseEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test-a", Value: "test-value-a"}},
290+
overwriteEnvVars: []corev1.EnvVar{corev1.EnvVar{Name: "test-b", Value: "test-value-b"}},
291+
wantEnv: []corev1.EnvVar{
292+
corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
293+
corev1.EnvVar{Name: "test-b", Value: "test-value-b"},
294+
},
295+
},
296+
{
297+
description: "Env var in overwrite slice overwrites expected env var from base slice",
298+
baseEnvVars: []corev1.EnvVar{
299+
corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
300+
corev1.EnvVar{Name: "test-b", Value: "test-value-b"},
301+
},
302+
overwriteEnvVars: []corev1.EnvVar{
303+
corev1.EnvVar{Name: "test-b", Value: "test-value-d"},
304+
corev1.EnvVar{Name: "test-c", Value: "test-value-c"},
305+
},
306+
wantEnv: []corev1.EnvVar{
307+
corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
308+
corev1.EnvVar{Name: "test-b", Value: "test-value-d"},
309+
corev1.EnvVar{Name: "test-c", Value: "test-value-c"},
310+
},
311+
},
312+
{
313+
description: "Multiple env vars in both slices returns env vars in sorted order",
314+
baseEnvVars: []corev1.EnvVar{
315+
corev1.EnvVar{Name: "a", Value: "test-value-a"},
316+
corev1.EnvVar{Name: "c", Value: "test-value-c"},
317+
},
318+
overwriteEnvVars: []corev1.EnvVar{
319+
corev1.EnvVar{Name: "d", Value: "test-value-d"},
320+
corev1.EnvVar{Name: "b", Value: "test-value-b"},
321+
},
322+
wantEnv: []corev1.EnvVar{
323+
corev1.EnvVar{Name: "a", Value: "test-value-a"},
324+
corev1.EnvVar{Name: "b", Value: "test-value-b"},
325+
corev1.EnvVar{Name: "c", Value: "test-value-c"},
326+
corev1.EnvVar{Name: "d", Value: "test-value-d"},
327+
},
328+
},
329+
{
330+
description: "Duplicate env var keys in base slice returns error",
331+
baseEnvVars: []corev1.EnvVar{
332+
corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
333+
corev1.EnvVar{Name: "test-a", Value: "test-value-b"},
334+
},
335+
overwriteEnvVars: []corev1.EnvVar{},
336+
wantErr: cmpopts.AnyError,
337+
},
338+
{
339+
description: "Duplicate env var keys in overwrite slice returns error",
340+
baseEnvVars: []corev1.EnvVar{},
341+
overwriteEnvVars: []corev1.EnvVar{
342+
corev1.EnvVar{Name: "test-a", Value: "test-value-a"},
343+
corev1.EnvVar{Name: "test-a", Value: "test-value-b"},
344+
},
345+
wantErr: cmpopts.AnyError,
346+
},
347+
}
348+
for _, testCase := range testCases {
349+
t.Run(testCase.description, func(t *testing.T) {
350+
gotEnv, err := mergeEnvVars(testCase.baseEnvVars, testCase.overwriteEnvVars)
351+
if got, want := gotEnv, testCase.wantEnv; !cmp.Equal(got, want) {
352+
t.Errorf("mergeEnvVars(%+v, %+v): unexpected env slice: got %+v, want %+v", testCase.baseEnvVars, testCase.overwriteEnvVars, got, want)
353+
}
354+
if got, want := err, testCase.wantErr; !cmp.Equal(got, want, cmpopts.EquateErrors()) {
355+
t.Errorf("mergeEnvVars(%+v, %+v): unexpected env slice: got %v, want %v", testCase.baseEnvVars, testCase.overwriteEnvVars, got, want)
356+
}
357+
})
358+
}
359+
}

‎controllers/testdata/appfabric.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,14 @@
7373
"--env=k8s"
7474
],
7575
"env": [
76+
{
77+
"name": "all-services-test",
78+
"value": "some-value-overridden"
79+
},
80+
{
81+
"name": "appfabric-env-var-test",
82+
"value": "some-value"
83+
},
7684
{
7785
"name": "JAVA_HEAPMAX",
7886
"value": "-Xmx62914560"

‎controllers/testdata/artifactcache.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,10 @@
208208
"--env=k8s"
209209
],
210210
"env":[
211+
{
212+
"name": "all-services-test",
213+
"value": "some-value"
214+
},
211215
{
212216
"name":"JAVA_HEAPMAX",
213217
"value":"-Xmx125829120"

‎controllers/testdata/authentication.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@
7373
"--env=k8s"
7474
],
7575
"env": [
76+
{
77+
"name": "all-services-test",
78+
"value": "some-value"
79+
},
7680
{
7781
"name": "JAVA_HEAPMAX",
7882
"value": "-Xmx62914560"

0 commit comments

Comments
 (0)