From 1e56149c917428e2fe3b015ac2745e6fcb303002 Mon Sep 17 00:00:00 2001 From: haibzhou Date: Wed, 27 Nov 2019 11:58:01 -0700 Subject: [PATCH] config: allow init containers if not decorated When the decorate is set as false, I think we can allow init containers to be used. This is useful if we want to run prow where we can't rely on gcs. We would still be able to configure our own containers to pull code and ship logs, etc. --- prow/config/config.go | 30 ++++++++++++++-------- prow/config/config_test.go | 52 ++++++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 24 deletions(-) diff --git a/prow/config/config.go b/prow/config/config.go index 3ea727f94620..023474330562 100644 --- a/prow/config/config.go +++ b/prow/config/config.go @@ -1081,7 +1081,7 @@ func validateJobBase(v JobBase, jobType prowapi.ProwJobType, podNamespace string if err := validateAgent(v, podNamespace); err != nil { return err } - if err := validatePodSpec(jobType, v.Spec); err != nil { + if err := validatePodSpec(jobType, v.Decorate, v.Spec); err != nil { return err } if err := ValidatePipelineRunSpec(jobType, v.ExtraRefs, v.PipelineRunSpec); err != nil { @@ -1644,28 +1644,36 @@ func ValidatePipelineRunSpec(jobType prowapi.ProwJobType, extraRefs []prowapi.Re return nil } -func validatePodSpec(jobType prowapi.ProwJobType, spec *v1.PodSpec) error { +func validatePodSpec(jobType prowapi.ProwJobType, decorated bool, spec *v1.PodSpec) error { if spec == nil { return nil } - if len(spec.InitContainers) != 0 { - return errors.New("pod spec may not use init containers") + if len(spec.InitContainers) != 0 && decorated { + return errors.New("pod spec may not use init containers when decorate is true") } - if n := len(spec.Containers); n != 1 { - return fmt.Errorf("pod spec must specify exactly 1 container, found: %d", n) + if n := len(spec.Containers); n != 1 && decorated { + return fmt.Errorf("pod spec must specify exactly 1 container when decorate is true, found: %d", n) + } else if n == 0 { + return fmt.Errorf("pod spec must specify at leaset 1 container, found: %d", n) } - for _, env := range spec.Containers[0].Env { - for _, prowEnv := range downwardapi.EnvForType(jobType) { - if env.Name == prowEnv { - // TODO(fejta): consider allowing this - return fmt.Errorf("env %s is reserved", env.Name) + for _, container := range spec.Containers { + for _, env := range container.Env { + for _, prowEnv := range downwardapi.EnvForType(jobType) { + if env.Name == prowEnv { + // TODO(fejta): consider allowing this + return fmt.Errorf("env %s is reserved", env.Name) + } } } } + if !decorated { + return nil + } + for _, mount := range spec.Containers[0].VolumeMounts { for _, prowMount := range decorate.VolumeMounts() { if mount.Name == prowMount { diff --git a/prow/config/config_test.go b/prow/config/config_test.go index a4e3b2483f15..7d2a22bcc259 100644 --- a/prow/config/config_test.go +++ b/prow/config/config_test.go @@ -542,11 +542,12 @@ func TestValidatePodSpec(t *testing.T) { postEnv := sets.NewString(downwardapi.EnvForType(prowapi.PostsubmitJob)...) preEnv := sets.NewString(downwardapi.EnvForType(prowapi.PresubmitJob)...) cases := []struct { - name string - jobType prowapi.ProwJobType - spec func(s *v1.PodSpec) - noSpec bool - pass bool + name string + decorate bool + jobType prowapi.ProwJobType + spec func(s *v1.PodSpec) + noSpec bool + pass bool }{ { name: "allow nil spec", @@ -558,13 +559,24 @@ func TestValidatePodSpec(t *testing.T) { pass: true, }, { - name: "reject init containers", + name: "reject init containers", + decorate: true, spec: func(s *v1.PodSpec) { s.InitContainers = []v1.Container{ {}, } }, }, + { + name: "allow init containers if not decorated", + decorate: false, + spec: func(s *v1.PodSpec) { + s.InitContainers = []v1.Container{ + {}, + } + }, + pass: true, + }, { name: "reject 0 containers", spec: func(s *v1.PodSpec) { @@ -572,11 +584,20 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject 2 containers", + name: "reject 2 containers", + decorate: true, spec: func(s *v1.PodSpec) { s.Containers = append(s.Containers, v1.Container{}) }, }, + { + name: "allow 2 containers if not decorated", + decorate: false, + spec: func(s *v1.PodSpec) { + s.Containers = append(s.Containers, v1.Container{}) + }, + pass: true, + }, { name: "reject reserved presubmit env", jobType: prowapi.PresubmitJob, @@ -620,7 +641,8 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject reserved mount name", + name: "reject reserved mount name", + decorate: true, spec: func(s *v1.PodSpec) { s.Containers[0].VolumeMounts = append(s.Containers[0].VolumeMounts, v1.VolumeMount{ Name: decorate.VolumeMounts()[0], @@ -629,7 +651,8 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject reserved mount path", + name: "reject reserved mount path", + decorate: true, spec: func(s *v1.PodSpec) { s.Containers[0].VolumeMounts = append(s.Containers[0].VolumeMounts, v1.VolumeMount{ Name: "fun", @@ -638,7 +661,8 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject conflicting mount paths (decorate in user)", + name: "reject conflicting mount paths (decorate in user)", + decorate: true, spec: func(s *v1.PodSpec) { s.Containers[0].VolumeMounts = append(s.Containers[0].VolumeMounts, v1.VolumeMount{ Name: "foo", @@ -647,7 +671,8 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject conflicting mount paths (user in decorate)", + name: "reject conflicting mount paths (user in decorate)", + decorate: true, spec: func(s *v1.PodSpec) { s.Containers[0].VolumeMounts = append(s.Containers[0].VolumeMounts, v1.VolumeMount{ Name: "foo", @@ -656,7 +681,8 @@ func TestValidatePodSpec(t *testing.T) { }, }, { - name: "reject reserved volume", + name: "reject reserved volume", + decorate: true, spec: func(s *v1.PodSpec) { s.Volumes = append(s.Volumes, v1.Volume{Name: decorate.VolumeMounts()[0]}) }, @@ -681,7 +707,7 @@ func TestValidatePodSpec(t *testing.T) { } else if tc.spec != nil { tc.spec(current) } - switch err := validatePodSpec(jt, current); { + switch err := validatePodSpec(jt, tc.decorate, current); { case err == nil && !tc.pass: t.Error("validation failed to raise an error") case err != nil && tc.pass: