diff --git a/hack/runtests.sh b/hack/runtests.sh index 19a30389..440cc7af 100755 --- a/hack/runtests.sh +++ b/hack/runtests.sh @@ -1,22 +1,25 @@ #!/bin/bash -set -euxo pipefail +set -exo pipefail -if [ ! -f ${KUBECONFIG} ] +if [ ! -z "${KUBECONFIG}" ] then - unset KUBECONFIG -else - K="kubectl --kubeconfig $KUBECONFIG --namespace default" - if $K get configmap ok-to-destroy + if [ ! -f "${KUBECONFIG}" ] then - set +e # do not break if crd not found - $K delete functions --all - $K delete environments --all - $K delete httptriggers --all - $K delete kuberneteswatchtriggers --all - $K delete messagequeuetriggers --all - $K delete packages --all - $K delete timetriggers --all - set -e + unset KUBECONFIG + else + K="kubectl --kubeconfig $KUBECONFIG --namespace default" + if $K get configmap ok-to-destroy + then + set +e # do not break if crd not found + $K delete functions --all + $K delete environments --all + $K delete httptriggers --all + $K delete kuberneteswatchtriggers --all + $K delete messagequeuetriggers --all + $K delete packages --all + $K delete timetriggers --all + set -e + fi fi fi @@ -26,8 +29,8 @@ set +x echo "" > coverage.txt # The executor unit test only works with NodePort-type services for -# now. So disable it for our travis ci tests. -for d in $(go list ./... | grep -v '/vendor/' | grep -v 'examples/go' | grep -v executor | grep -v 'benchmark'); do +# now. So disable it for our travis ci tests except some partial tests. +for d in $(go list ./... | grep -v '/vendor/' | grep -v 'examples/go' | grep -v executor | grep -v 'benchmark') github.com/fission/fission/pkg/executor/util; do go test -v -coverprofile=profile.out -covermode=atomic $d if [ -f profile.out ]; then cat profile.out >> coverage.txt diff --git a/pkg/buildermgr/envwatcher.go b/pkg/buildermgr/envwatcher.go index ffda7e68..bfcfdfc3 100644 --- a/pkg/buildermgr/envwatcher.go +++ b/pkg/buildermgr/envwatcher.go @@ -465,6 +465,30 @@ func (envw *environmentWatcher) createBuilderDeployment(env *fv1.Environment, ns podAnnotations["sidecar.istio.io/inject"] = "false" } + container, err := util.MergeContainer(&apiv1.Container{ + Name: "builder", + Image: env.Spec.Builder.Image, + ImagePullPolicy: envw.builderImagePullPolicy, + TerminationMessagePath: "/dev/termination-log", + Command: []string{"/builder", envw.fetcherConfig.SharedMountPath()}, + ReadinessProbe: &apiv1.Probe{ + InitialDelaySeconds: 5, + PeriodSeconds: 2, + Handler: apiv1.Handler{ + HTTPGet: &apiv1.HTTPGetAction{ + Path: "/healthz", + Port: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 8001, + }, + }, + }, + }, + }, env.Spec.Builder.Container) + if err != nil { + return nil, err + } + deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Namespace: ns, @@ -482,35 +506,14 @@ func (envw *environmentWatcher) createBuilderDeployment(env *fv1.Environment, ns Annotations: podAnnotations, }, Spec: apiv1.PodSpec{ - Containers: []apiv1.Container{ - util.MergeContainerSpecs(&apiv1.Container{ - Name: "builder", - Image: env.Spec.Builder.Image, - ImagePullPolicy: envw.builderImagePullPolicy, - TerminationMessagePath: "/dev/termination-log", - Command: []string{"/builder", envw.fetcherConfig.SharedMountPath()}, - ReadinessProbe: &apiv1.Probe{ - InitialDelaySeconds: 5, - PeriodSeconds: 2, - Handler: apiv1.Handler{ - HTTPGet: &apiv1.HTTPGetAction{ - Path: "/healthz", - Port: intstr.IntOrString{ - Type: intstr.Int, - IntVal: 8001, - }, - }, - }, - }, - }, env.Spec.Builder.Container), - }, + Containers: []apiv1.Container{*container}, ServiceAccountName: "fission-builder", }, }, }, } - err := envw.fetcherConfig.AddFetcherToPodSpec(&deployment.Spec.Template.Spec, "builder") + err = envw.fetcherConfig.AddFetcherToPodSpec(&deployment.Spec.Template.Spec, "builder") if err != nil { return nil, err } diff --git a/pkg/executor/newdeploy/newdeploy.go b/pkg/executor/newdeploy/newdeploy.go index ed286f1f..6f872419 100644 --- a/pkg/executor/newdeploy/newdeploy.go +++ b/pkg/executor/newdeploy/newdeploy.go @@ -197,6 +197,40 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen // rollback, set RevisionHistoryLimit to 0 to disable this feature. revisionHistoryLimit := int32(0) + container, err := util.MergeContainer(&apiv1.Container{ + Name: fn.Metadata.Name, + Image: env.Spec.Runtime.Image, + ImagePullPolicy: deploy.runtimeImagePullPolicy, + TerminationMessagePath: "/dev/termination-log", + Lifecycle: &apiv1.Lifecycle{ + PreStop: &apiv1.Handler{ + Exec: &apiv1.ExecAction{ + Command: []string{ + "/bin/sleep", + fmt.Sprintf("%v", gracePeriodSeconds), + }, + }, + }, + }, + Env: []apiv1.EnvVar{ + { + Name: fv1.LastUpdateTimestamp, + Value: time.Now().String(), + }, + }, + // https://istio.io/docs/setup/kubernetes/additional-setup/requirements/ + Ports: []apiv1.ContainerPort{ + { + Name: "http-env", + ContainerPort: int32(8888), + }, + }, + Resources: resources, + }, env.Spec.Runtime.Container) + if err != nil { + return nil, err + } + deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: deployName, @@ -213,38 +247,7 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen Annotations: podAnnotations, }, Spec: apiv1.PodSpec{ - Containers: []apiv1.Container{ - util.MergeContainerSpecs(&apiv1.Container{ - Name: fn.Metadata.Name, - Image: env.Spec.Runtime.Image, - ImagePullPolicy: deploy.runtimeImagePullPolicy, - TerminationMessagePath: "/dev/termination-log", - Lifecycle: &apiv1.Lifecycle{ - PreStop: &apiv1.Handler{ - Exec: &apiv1.ExecAction{ - Command: []string{ - "/bin/sleep", - fmt.Sprintf("%v", gracePeriodSeconds), - }, - }, - }, - }, - Env: []apiv1.EnvVar{ - { - Name: fv1.LastUpdateTimestamp, - Value: time.Now().String(), - }, - }, - // https://istio.io/docs/setup/kubernetes/additional-setup/requirements/ - Ports: []apiv1.ContainerPort{ - { - Name: "http-env", - ContainerPort: int32(8888), - }, - }, - Resources: resources, - }, env.Spec.Runtime.Container), - }, + Containers: []apiv1.Container{*container}, ServiceAccountName: "fission-fetcher", TerminationGracePeriodSeconds: &gracePeriodSeconds, }, @@ -261,7 +264,7 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen } // Order of merging is important here - first fetcher, then containers and lastly pod spec - err := deploy.fetcherConfig.AddSpecializingFetcherToPodSpec( + err = deploy.fetcherConfig.AddSpecializingFetcherToPodSpec( &deployment.Spec.Template.Spec, fn.Metadata.Name, fn, @@ -272,10 +275,11 @@ func (deploy *NewDeploy) getDeploymentSpec(fn *fv1.Function, env *fv1.Environmen } if env.Spec.Runtime.PodSpec != nil { - err := util.MergePodSpec(&deployment.Spec.Template.Spec, env.Spec.Runtime.PodSpec) + newPodSpec, err := util.MergePodSpec(&deployment.Spec.Template.Spec, env.Spec.Runtime.PodSpec) if err != nil { return nil, err } + deployment.Spec.Template.Spec = *newPodSpec } return deployment, nil diff --git a/pkg/executor/poolmgr/gp.go b/pkg/executor/poolmgr/gp.go index eb64905e..2befc087 100644 --- a/pkg/executor/poolmgr/gp.go +++ b/pkg/executor/poolmgr/gp.go @@ -359,6 +359,44 @@ func (gp *GenericPool) createPool() error { podAnnotations["sidecar.istio.io/inject"] = "false" } + container, err := util.MergeContainer(&apiv1.Container{ + Name: gp.env.Metadata.Name, + Image: gp.env.Spec.Runtime.Image, + ImagePullPolicy: gp.runtimeImagePullPolicy, + TerminationMessagePath: "/dev/termination-log", + Resources: gp.env.Spec.Resources, + // Pod is removed from endpoints list for service when it's + // state became "Termination". We used preStop hook as the + // workaround for connection draining since pod maybe shutdown + // before grace period expires. + // https://kubernetes.io/docs/concepts/workloads/pods/pod/#termination-of-pods + // https://github.com/kubernetes/kubernetes/issues/47576#issuecomment-308900172 + Lifecycle: &apiv1.Lifecycle{ + PreStop: &apiv1.Handler{ + Exec: &apiv1.ExecAction{ + Command: []string{ + "/bin/sleep", + fmt.Sprintf("%v", gracePeriodSeconds), + }, + }, + }, + }, + // https://istio.io/docs/setup/kubernetes/additional-setup/requirements/ + Ports: []apiv1.ContainerPort{ + { + Name: "http-fetcher", + ContainerPort: int32(8000), + }, + { + Name: "http-env", + ContainerPort: int32(8888), + }, + }, + }, gp.env.Spec.Runtime.Container) + if err != nil { + return err + } + deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: gp.getPoolName(), @@ -375,42 +413,7 @@ func (gp *GenericPool) createPool() error { Annotations: podAnnotations, }, Spec: apiv1.PodSpec{ - Containers: []apiv1.Container{ - util.MergeContainerSpecs(&apiv1.Container{ - Name: gp.env.Metadata.Name, - Image: gp.env.Spec.Runtime.Image, - ImagePullPolicy: gp.runtimeImagePullPolicy, - TerminationMessagePath: "/dev/termination-log", - Resources: gp.env.Spec.Resources, - // Pod is removed from endpoints list for service when it's - // state became "Termination". We used preStop hook as the - // workaround for connection draining since pod maybe shutdown - // before grace period expires. - // https://kubernetes.io/docs/concepts/workloads/pods/pod/#termination-of-pods - // https://github.com/kubernetes/kubernetes/issues/47576#issuecomment-308900172 - Lifecycle: &apiv1.Lifecycle{ - PreStop: &apiv1.Handler{ - Exec: &apiv1.ExecAction{ - Command: []string{ - "/bin/sleep", - fmt.Sprintf("%v", gracePeriodSeconds), - }, - }, - }, - }, - // https://istio.io/docs/setup/kubernetes/additional-setup/requirements/ - Ports: []apiv1.ContainerPort{ - { - Name: "http-fetcher", - ContainerPort: int32(8000), - }, - { - Name: "http-env", - ContainerPort: int32(8888), - }, - }, - }, gp.env.Spec.Runtime.Container), - }, + Containers: []apiv1.Container{*container}, ServiceAccountName: "fission-fetcher", // TerminationGracePeriodSeconds should be equal to the // sleep time of preStop to make sure that SIGTERM is sent @@ -422,16 +425,17 @@ func (gp *GenericPool) createPool() error { } // Order of merging is important here - first fetcher, then containers and lastly pod spec - err := gp.fetcherConfig.AddFetcherToPodSpec(&deployment.Spec.Template.Spec, gp.env.Metadata.Name) + err = gp.fetcherConfig.AddFetcherToPodSpec(&deployment.Spec.Template.Spec, gp.env.Metadata.Name) if err != nil { return err } if gp.env.Spec.Runtime.PodSpec != nil { - err = util.MergePodSpec(&deployment.Spec.Template.Spec, gp.env.Spec.Runtime.PodSpec) + newPodSpec, err := util.MergePodSpec(&deployment.Spec.Template.Spec, gp.env.Spec.Runtime.PodSpec) if err != nil { return err } + deployment.Spec.Template.Spec = *newPodSpec } depl, err := gp.kubernetesClient.AppsV1().Deployments(gp.namespace).Create(deployment) diff --git a/pkg/executor/util/merge.go b/pkg/executor/util/merge.go index ecf58520..85bbab6b 100644 --- a/pkg/executor/util/merge.go +++ b/pkg/executor/util/merge.go @@ -17,81 +17,73 @@ limitations under the License. package util import ( - "errors" + "fmt" + "reflect" "github.com/hashicorp/go-multierror" "github.com/imdario/mergo" apiv1 "k8s.io/api/core/v1" ) -// MergeContainerSpecs merges container specs using a predefined order. -// -// The order of the arguments indicates which spec has precedence (lower index takes precedence over higher indexes). -// Slices and maps are merged; other fields are set only if they are a zero value. -func MergeContainerSpecs(specs ...*apiv1.Container) apiv1.Container { - result := &apiv1.Container{} - for _, spec := range specs { - if spec == nil { - continue - } +// TODO: replace functions here with native kubernetes strategic merge patch. +// https://kubernetes.io/docs/tasks/run-application/update-api-object-kubectl-patch/#use-a-strategic-merge-patch-to-update-a-deployment - err := mergo.Merge(result, spec) - if err != nil { - panic(err) - } +// MergeContainer returns merged container specs. +// Slices are merged, and return an error if the elements in the slice have name conflicts. +// Maps are merged, the value of map of dst container are overridden if the key is the same. +// The rest of fields of dst container are overridden directly. +func MergeContainer(dst *apiv1.Container, src *apiv1.Container) (*apiv1.Container, error) { + if src == nil { + return dst, nil } - return *result + + // to prevent any modification to the original obj + dstC := *dst + + errs := &multierror.Error{} + err := mergo.Merge(&dstC, src, mergo.WithAppendSlice, mergo.WithOverride) + if err != nil { + return nil, err + } + errs = multierror.Append(errs, + checkSliceConflicts("Name", dstC.Ports), + checkSliceConflicts("Name", dstC.Env), + checkSliceConflicts("Name", dstC.VolumeMounts), + checkSliceConflicts("Name", dstC.VolumeDevices)) + + return &dstC, errs.ErrorOrNil() } -// mergeContainer is a specialized implementation of MergeContainerSpecs -func mergeContainer(deployContainer *apiv1.Container, containerSpec apiv1.Container) error { - - if &containerSpec == nil { - return nil - } - - if deployContainer.Name == containerSpec.Name { - volMap := make(map[string]*apiv1.VolumeMount) - for _, vol := range deployContainer.VolumeMounts { - volMap[vol.Name] = &vol - } - for _, specVol := range containerSpec.VolumeMounts { - _, ok := volMap[specVol.Name] - if ok { - return errors.New("Duplicate volume name found in the spec") - } else { - deployContainer.VolumeMounts = append(deployContainer.VolumeMounts, specVol) - } - } - deployContainer.Env = append(deployContainer.Env, containerSpec.Env...) - } - return nil -} - -func MergePodSpec(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) error { - if &targetPodSpec == nil { - return nil +func MergePodSpec(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) (*apiv1.PodSpec, error) { + if targetPodSpec == nil { + return srcPodSpec, nil } multierr := &multierror.Error{} // Get item from spec, if they exist in deployment - merge, else append // Same pattern for all lists (Mergo can not handle lists) - // At some point this is better done with generics/reflection? - err := mergeContainerLists(srcPodSpec, targetPodSpec) + // TODO: At some point this is better done with generics/reflection? + cList, err := mergeContainerList(srcPodSpec.Containers, targetPodSpec.Containers) if err != nil { - return err + multierr = multierror.Append(multierr, err) + } else { + srcPodSpec.Containers = cList } - err = mergeInitContainerList(srcPodSpec, targetPodSpec) + cList, err = mergeContainerList(srcPodSpec.InitContainers, targetPodSpec.InitContainers) if err != nil { - return err + multierr = multierror.Append(multierr, err) + } else { + srcPodSpec.InitContainers = cList } // For volumes - if duplicate exist, throw error - err = mergeVolumeLists(srcPodSpec, targetPodSpec) + vols, err := mergeVolumeLists(srcPodSpec.Volumes, targetPodSpec.Volumes) if err != nil { - return err + multierr = multierror.Append(multierr, err) + } else { + srcPodSpec.Volumes = vols } if targetPodSpec.NodeName != "" { @@ -145,75 +137,96 @@ func MergePodSpec(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) error multierr = multierror.Append(multierr, err) } - return multierr.ErrorOrNil() + return srcPodSpec, multierr.ErrorOrNil() } -func mergeContainerLists(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) error { - targetSpecContainers := targetPodSpec.Containers - targetContainers := make(map[string]apiv1.Container) - for _, c := range targetSpecContainers { - targetContainers[c.Name] = c - } +func mergeContainerList(dst []apiv1.Container, src []apiv1.Container) ([]apiv1.Container, error) { + errs := &multierror.Error{} - multierr := &multierror.Error{} - for _, c := range srcPodSpec.Containers { - container, ok := targetContainers[c.Name] + list := append(dst, src...) + containers := make(map[string]*apiv1.Container, len(list)) + + for i, c := range list { + container, ok := containers[c.Name] if ok { - err := mergeContainer(&c, container) - multierr = multierror.Append(multierr, err) - delete(targetContainers, c.Name) - } - } - - for _, container := range targetContainers { - srcPodSpec.Containers = append(srcPodSpec.Containers, container) - } - - return multierr.ErrorOrNil() -} - -func mergeInitContainerList(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) error { - targetSpecContainers := targetPodSpec.InitContainers - targetContainers := make(map[string]apiv1.Container) - for _, c := range targetSpecContainers { - targetContainers[c.Name] = c - } - - multierr := &multierror.Error{} - for _, c := range srcPodSpec.InitContainers { - container, ok := targetContainers[c.Name] - if ok { - err := mergeContainer(&c, container) - multierr = multierror.Append(multierr, err) - delete(targetContainers, c.Name) - } - } - - for _, container := range targetContainers { - srcPodSpec.InitContainers = append(srcPodSpec.InitContainers, container) - } - return multierr.ErrorOrNil() -} - -func mergeVolumeLists(srcPodSpec *apiv1.PodSpec, targetPodSpec *apiv1.PodSpec) error { - volumeList := targetPodSpec.Volumes - specVolumes := make(map[string]apiv1.Volume) - for _, vol := range volumeList { - specVolumes[vol.Name] = vol - } - - multierr := &multierror.Error{} - for _, vol := range srcPodSpec.Volumes { - _, ok := specVolumes[vol.Name] - if ok { - multierr = multierror.Append(multierr, errors.New("Duplicate volume name found in the spec")) + newC, err := MergeContainer(container, &c) + if err != nil { + // record the error and continue + errs = multierror.Append(errs, err) + } else { + containers[c.Name] = newC + } } else { - delete(specVolumes, vol.Name) + containers[c.Name] = &list[i] } } - for _, volume := range specVolumes { - srcPodSpec.Volumes = append(srcPodSpec.Volumes, volume) + var containerList []apiv1.Container + for _, c := range containers { + containerList = append(containerList, *c) } - return multierr.ErrorOrNil() + + if errs.ErrorOrNil() != nil { + return nil, errs.ErrorOrNil() + } + + return containerList, nil +} + +func mergeVolumeLists(dst []apiv1.Volume, src []apiv1.Volume) ([]apiv1.Volume, error) { + dst = append(dst, src...) + err := checkSliceConflicts("Name", dst) + if err != nil { + return nil, err + } + return dst, err +} + +func checkSliceConflicts(field string, objs interface{}) (err error) { + defer func() { + // just in case to recover from unknown error + if e := recover(); e != nil { + err = fmt.Errorf("error checking slice conflicts: %v", e) + } + }() + + if reflect.TypeOf(objs).Kind() != reflect.Slice { + return fmt.Errorf("not a slice type: %v", reflect.TypeOf(objs)) + } + + errs := &multierror.Error{} + names := make(map[string]struct{}) + + s := reflect.ValueOf(objs) + var elemType reflect.Type + + for i := 0; i < s.Len(); i++ { + r := s.Index(i) + + // if objs pass in is a slice of interface{} ([]interface{}), then + // use Elem() to get element value. + if r.Kind() == reflect.Interface { + r = r.Elem() + } + objType := reflect.Indirect(r).Type() + + if elemType == nil { + elemType = objType + } else if objType != elemType { + return fmt.Errorf("unable to check conflict between different types: %v, %v", elemType, objType) + } + + f := reflect.Indirect(r).FieldByName(field) + if !f.IsValid() { + return fmt.Errorf("cannot compare type without target field: %v %v", objType, field) + } + + _, ok := names[f.String()] + if ok { + errs = multierror.Append(errs, fmt.Errorf("duplicate name in %v: %v", objType, f.String())) + } else { + names[f.String()] = struct{}{} + } + } + return errs.ErrorOrNil() } diff --git a/pkg/executor/util/merge_test.go b/pkg/executor/util/merge_test.go index f218c086..240f3f08 100644 --- a/pkg/executor/util/merge_test.go +++ b/pkg/executor/util/merge_test.go @@ -16,105 +16,318 @@ limitations under the License. package util import ( + "reflect" "testing" - "github.com/stretchr/testify/assert" apiv1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" ) -func TestMergeContainerSpecs(t *testing.T) { - expected := apiv1.Container{ - Name: "containerName", - Image: "testImage", - Command: []string{ - "command", +func Test_checkConflicts(t *testing.T) { + type args struct { + objs interface{} + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "container name", + args: args{[]apiv1.Container{{Name: "test1"}, {Name: "test2"}, {Name: "test3"}}}, + wantErr: false, }, - Args: []string{ - "arg1", - "arg2", + { + name: "pass non-slice", + args: args{apiv1.Container{Name: "test1"}}, + wantErr: true, }, - ImagePullPolicy: apiv1.PullNever, - TTY: true, - Env: []apiv1.EnvVar{ - { - Name: "a", - Value: "b", - }, - { - Name: "c", - Value: "d", - }, + { + name: "conflict container name", + args: args{[]interface{}{apiv1.Container{Name: "test1"}, apiv1.Container{Name: "test1"}, apiv1.Container{Name: "test3"}}}, + wantErr: true, + }, + { + name: "different types", + args: args{[]interface{}{apiv1.VolumeMount{Name: "test1"}, apiv1.EnvFromSource{Prefix: "", ConfigMapRef: nil, SecretRef: nil}}}, + wantErr: true, + }, + { + name: "type without target field", + args: args{[]interface{}{apiv1.EnvFromSource{Prefix: "", ConfigMapRef: nil, SecretRef: nil}}}, + wantErr: true, }, } - specs := []*apiv1.Container{ + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := checkSliceConflicts("Name", tt.args.objs); (err != nil) != tt.wantErr { + t.Errorf("checkNameConflict() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func Test_mergeContainer(t *testing.T) { + type args struct { + dst *apiv1.Container + src *apiv1.Container + } + tests := []struct { + name string + args args + want *apiv1.Container + wantErr bool + }{ { - Name: "containerName", - Image: "testImage", - Command: []string{ - "command", + name: "nil-src", + args: args{ + dst: &apiv1.Container{ + Name: "test", + }, + src: nil, }, - Args: []string{ - "arg1", - "arg2", + want: &apiv1.Container{ + Name: "test", }, - ImagePullPolicy: apiv1.PullNever, - TTY: true, + wantErr: false, }, { - Name: "shouldNotBeThere", - Image: "shouldNotBeThere", - Env: []apiv1.EnvVar{ - { - Name: "a", - Value: "b", + name: "normal-merge", + args: args{ + dst: &apiv1.Container{ + Name: "test", + Image: "foobar", + Command: []string{"a"}, + Args: []string{"b"}, + WorkingDir: "/tmp", + Ports: []apiv1.ContainerPort{{Name: "http1", HostPort: 123}}, + EnvFrom: []apiv1.EnvFromSource{{Prefix: "asd"}}, + Env: []apiv1.EnvVar{{Name: "foobar", Value: "dummy"}}, + Resources: apiv1.ResourceRequirements{Limits: apiv1.ResourceList{apiv1.ResourceCPU: resource.Quantity{Format: "limit"}}}, + VolumeMounts: []apiv1.VolumeMount{{Name: "volm", ReadOnly: true, MountPath: "/tmp/foobar"}}, + VolumeDevices: []apiv1.VolumeDevice{{Name: "vold", DevicePath: "hello"}}, + LivenessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 1, TimeoutSeconds: 2, PeriodSeconds: 3, SuccessThreshold: 4, FailureThreshold: 5}, + ReadinessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 1, TimeoutSeconds: 2, PeriodSeconds: 3, SuccessThreshold: 4, FailureThreshold: 5}, + ImagePullPolicy: "IfNotPresent", + }, + src: &apiv1.Container{ + Name: "test", + Image: "foobar-1", + Command: []string{"a", "c"}, + Args: []string{"b", "d"}, + WorkingDir: "/tmp/qwer", + Ports: []apiv1.ContainerPort{{Name: "http2", HostPort: 123}}, + EnvFrom: []apiv1.EnvFromSource{{Prefix: "asd"}}, + Env: []apiv1.EnvVar{{Name: "foobar1", Value: "dummy"}}, + Resources: apiv1.ResourceRequirements{Limits: apiv1.ResourceList{apiv1.ResourceCPU: resource.Quantity{Format: "unlimit"}, apiv1.ResourceMemory: resource.Quantity{Format: "limit"}}}, + VolumeMounts: []apiv1.VolumeMount{{Name: "volm1", ReadOnly: true, MountPath: "/tmp/foobar"}}, + VolumeDevices: []apiv1.VolumeDevice{{Name: "vold1", DevicePath: "hello"}}, + LivenessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 5, TimeoutSeconds: 4, PeriodSeconds: 3, SuccessThreshold: 2, FailureThreshold: 1}, + ReadinessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 5, TimeoutSeconds: 4, PeriodSeconds: 3, SuccessThreshold: 2, FailureThreshold: 1}, + ImagePullPolicy: "Always", }, }, - ImagePullPolicy: apiv1.PullAlways, - TTY: false, + want: &apiv1.Container{ + Name: "test", + Image: "foobar-1", + Command: []string{"a", "a", "c"}, + Args: []string{"b", "b", "d"}, + WorkingDir: "/tmp/qwer", + Ports: []apiv1.ContainerPort{{Name: "http1", HostPort: 123}, {Name: "http2", HostPort: 123}}, + EnvFrom: []apiv1.EnvFromSource{{Prefix: "asd"}, {Prefix: "asd"}}, + Env: []apiv1.EnvVar{{Name: "foobar", Value: "dummy"}, {Name: "foobar1", Value: "dummy"}}, + Resources: apiv1.ResourceRequirements{Limits: apiv1.ResourceList{apiv1.ResourceCPU: resource.Quantity{Format: "unlimit"}, apiv1.ResourceMemory: resource.Quantity{Format: "limit"}}}, + VolumeMounts: []apiv1.VolumeMount{{Name: "volm", ReadOnly: true, MountPath: "/tmp/foobar"}, {Name: "volm1", ReadOnly: true, MountPath: "/tmp/foobar"}}, + VolumeDevices: []apiv1.VolumeDevice{{Name: "vold", DevicePath: "hello"}, {Name: "vold1", DevicePath: "hello"}}, + LivenessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 5, TimeoutSeconds: 4, PeriodSeconds: 3, SuccessThreshold: 2, FailureThreshold: 1}, + ReadinessProbe: &apiv1.Probe{Handler: apiv1.Handler{}, InitialDelaySeconds: 5, TimeoutSeconds: 4, PeriodSeconds: 3, SuccessThreshold: 2, FailureThreshold: 1}, + Lifecycle: nil, + TerminationMessagePath: "", + TerminationMessagePolicy: "", + ImagePullPolicy: "Always", + SecurityContext: nil, + Stdin: false, + StdinOnce: false, + TTY: false, + }, + wantErr: false, }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := MergeContainer(tt.args.dst, tt.args.src) + if (err != nil) != tt.wantErr { + t.Errorf("mergeContainer() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("mergeContainer() got = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_mergeVolumeLists(t *testing.T) { + type args struct { + dst []apiv1.Volume + src []apiv1.Volume + } + tests := []struct { + name string + args args + want []apiv1.Volume + wantErr bool + }{ { - Env: []apiv1.EnvVar{ - { - Name: "c", - Value: "d", + name: "merge volume list", + args: args{ + dst: []apiv1.Volume{ + {Name: "vol1", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foo"}}}, + {Name: "vol2", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/bar"}}}, + }, + src: []apiv1.Volume{ + {Name: "vol3", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foobar"}}}, }, }, - ImagePullPolicy: apiv1.PullIfNotPresent, - TTY: false, + want: []apiv1.Volume{ + {Name: "vol1", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foo"}}}, + {Name: "vol2", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/bar"}}}, + {Name: "vol3", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foobar"}}}, + }, + wantErr: false, + }, + { + name: "conflict volume name", + args: args{ + dst: []apiv1.Volume{ + {Name: "vol1", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foo"}}}, + {Name: "vol2", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/bar"}}}, + }, + src: []apiv1.Volume{ + {Name: "vol1", VolumeSource: apiv1.VolumeSource{HostPath: &apiv1.HostPathVolumeSource{Path: "/tmp/foobar"}}}, + }, + }, + want: nil, + wantErr: true, }, } - result := MergeContainerSpecs(specs...) - assert.Equal(t, expected, result) - - // Check if merging order actually matters - var rspecs []*apiv1.Container - for i := len(specs) - 1; i >= 0; i -= 1 { - rspecs = append(rspecs, specs[i]) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := mergeVolumeLists(tt.args.dst, tt.args.src) + if (err != nil) != tt.wantErr { + t.Errorf("mergeVolumeLists() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("mergeVolumeLists() got = %v, want %v", got, tt.want) + } + }) } - reverseResult := MergeContainerSpecs(rspecs...) - assert.NotEqual(t, expected, reverseResult) } -func TestMergeContainerSpecsSingle(t *testing.T) { - expected := apiv1.Container{ - Name: "containerName", - Image: "testImage", - Command: []string{ - "command", - }, - Args: []string{ - "arg1", - "arg2", - }, - ImagePullPolicy: apiv1.PullNever, - TTY: true, +func Test_mergeContainerList(t *testing.T) { + type args struct { + dst []apiv1.Container + src []apiv1.Container } - result := MergeContainerSpecs(&expected) - assert.EqualValues(t, expected, result) -} + tests := []struct { + name string + args args + want []apiv1.Container + wantErr bool + }{ + { + name: "merge container with conflict name", + args: args{ + dst: []apiv1.Container{ + {Name: "foo", Image: "dummy-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo2", Image: "dummy-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + }, + src: []apiv1.Container{ + {Name: "foo", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "test", Value: "foobar"}}}, + {Name: "foo2", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + }, + want: []apiv1.Container{ + {Name: "foo", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}, {Name: "test", Value: "foobar"}}}, + {Name: "foo2", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}, {Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + wantErr: false, + }, + { + name: "merge container with no conflict name", + args: args{ + dst: []apiv1.Container{ + {Name: "foo", Image: "dummy-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo2", Image: "dummy-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + }, + src: []apiv1.Container{ + {Name: "foo3", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "test", Value: "foobar"}}}, + {Name: "foo4", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + }, + want: []apiv1.Container{ + {Name: "foo", Image: "dummy-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo2", Image: "dummy-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo3", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "test", Value: "foobar"}}}, + {Name: "foo4", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + wantErr: false, + }, + { + name: "merge container with partial conflict name", + args: args{ + dst: []apiv1.Container{ + {Name: "foo", Image: "dummy-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo2", Image: "dummy-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + }, + src: []apiv1.Container{ + {Name: "foo", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "test", Value: "foobar"}}}, + {Name: "foo4", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + }, + want: []apiv1.Container{ + {Name: "foo", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}, {Name: "test", Value: "foobar"}}}, + {Name: "foo2", Image: "dummy-image-2", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + {Name: "foo4", Image: "my-custom-image-2", Env: []apiv1.EnvVar{{Name: "env3", Value: "foobar"}, {Name: "env4", Value: "barfoo"}}}, + }, + wantErr: false, + }, + { + name: "merge container with conflict environment variable", + args: args{ + dst: []apiv1.Container{ + {Name: "foo", Image: "dummy-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "foobar"}, {Name: "env2", Value: "barfoo"}}}, + }, + src: []apiv1.Container{ + {Name: "foo", Image: "my-custom-image-1", Env: []apiv1.EnvVar{{Name: "env1", Value: "helloworld"}}}, + }, + }, + want: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := mergeContainerList(tt.args.dst, tt.args.src) + if (err != nil) != tt.wantErr { + t.Errorf("mergeInitContainerList() error = %v, wantErr %v", err, tt.wantErr) + return + } -func TestMergeContainerSpecsNil(t *testing.T) { - expected := apiv1.Container{} - result := MergeContainerSpecs() - assert.EqualValues(t, expected, result) + for _, obj := range got { + match := false + for _, w := range tt.want { + if obj.Name == w.Name && reflect.DeepEqual(obj, w) { + match = true + break + } + } + if !match { + t.Errorf("mergeInitContainerList() got = %v, want %v", got, tt.want) + break + } + } + }) + } }