Skip to content

Commit 782008c

Browse files
ci-operator: allow users to depend on images in steps
Users may not expect to find the pull spec for an image created at some other point in the pipeline fed to their step via an environment variable that they specify. The ci-operator will ensure that their step only runs once the image is available. Signed-off-by: Steve Kuznetsov <[email protected]>
1 parent b219ef7 commit 782008c

File tree

11 files changed

+323
-16
lines changed

11 files changed

+323
-16
lines changed

pkg/api/config.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,7 @@ func validateLiteralTestStepCommon(fieldRoot string, step LiteralTestStep, seen
469469
if err := validateParameters(fieldRoot, step.Environment, env); err != nil {
470470
ret = append(ret, err)
471471
}
472+
ret = append(ret, validateDependencies(fieldRoot, step.Dependencies)...)
472473
return
473474
}
474475

@@ -551,6 +552,26 @@ func validateParameters(fieldRoot string, params []StepParameter, env TestEnviro
551552
return nil
552553
}
553554

555+
func validateDependencies(fieldRoot string, dependencies []StepDependency) []error {
556+
var errs []error
557+
env := sets.NewString()
558+
for i, dependency := range dependencies {
559+
if dependency.Name == "" {
560+
errs = append(errs, fmt.Errorf("%s.dependencies[%d].name must be set", fieldRoot, i))
561+
} else if strings.Contains(dependency.Name, ":") && len(strings.Split(dependency.Name, ":")) != 2 {
562+
errs = append(errs, fmt.Errorf("%s.dependencies[%d].name must take the `tag` or `stream:tag` form, not %q", fieldRoot, i, dependency.Name))
563+
}
564+
if dependency.Env == "" {
565+
errs = append(errs, fmt.Errorf("%s.dependencies[%d].env must be set", fieldRoot, i))
566+
} else if env.Has(dependency.Env) {
567+
errs = append(errs, fmt.Errorf("%s.dependencies[%d].env targets an environment variable that is already set by another dependency", fieldRoot, i))
568+
} else {
569+
env.Insert(dependency.Env)
570+
}
571+
}
572+
return errs
573+
}
574+
554575
func validateReleaseBuildConfiguration(input *ReleaseBuildConfiguration, org, repo string) []error {
555576
var validationErrors []error
556577

pkg/api/config_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1487,3 +1487,48 @@ func errListMessagesEqual(a, b []error) bool {
14871487
}
14881488
return true
14891489
}
1490+
1491+
func TestValidateDependencies(t *testing.T) {
1492+
var testCases = []struct {
1493+
name string
1494+
input []StepDependency
1495+
output []error
1496+
}{
1497+
{
1498+
name: "no dependencies",
1499+
input: nil,
1500+
},
1501+
{
1502+
name: "valid dependencies",
1503+
input: []StepDependency{
1504+
{Name: "src", Env: "SOURCE"},
1505+
{Name: "stable:installer", Env: "INSTALLER"},
1506+
},
1507+
},
1508+
{
1509+
name: "invalid dependencies",
1510+
input: []StepDependency{
1511+
{Name: "", Env: ""},
1512+
{Name: "src", Env: "SOURCE"},
1513+
{Name: "src", Env: "SOURCE"},
1514+
{Name: "src:lol:oops", Env: "WHOA"},
1515+
},
1516+
output: []error{
1517+
errors.New("root.dependencies[0].name must be set"),
1518+
errors.New("root.dependencies[0].env must be set"),
1519+
errors.New("root.dependencies[2].env targets an environment variable that is already set by another dependency"),
1520+
errors.New("root.dependencies[3].name must take the `tag` or `stream:tag` form, not \"src:lol:oops\""),
1521+
},
1522+
},
1523+
}
1524+
1525+
for _, testCase := range testCases {
1526+
t.Run(testCase.name, func(t *testing.T) {
1527+
if actual, expected := validateDependencies("root", testCase.input), testCase.output; !reflect.DeepEqual(actual, expected) {
1528+
t.Errorf("%s: got incorrect errors: %s", testCase.name, cmp.Diff(actual, expected, cmp.Comparer(func(x, y error) bool {
1529+
return x.Error() == y.Error()
1530+
})))
1531+
}
1532+
})
1533+
}
1534+
}

pkg/api/graph.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,12 @@ func IsReleaseStream(stream string) bool {
202202
return strings.HasPrefix(stream, StableImageStream)
203203
}
204204

205+
// IsReleasePayloadStream determines if the ImageStream holds
206+
// release paylaod images.
207+
func IsReleasePayloadStream(stream string) bool {
208+
return stream == ReleaseImageStream
209+
}
210+
205211
type StepNode struct {
206212
Step Step
207213
Children []*StepNode

pkg/api/types.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,9 @@ type LiteralTestStep struct {
563563
Credentials []CredentialReference `json:"credentials,omitempty"`
564564
// Environment lists parameters that should be set by the test.
565565
Environment []StepParameter `json:"env,omitempty"`
566+
// Dependencies lists images which must be available before the test runs
567+
// and the environment variables which are used to expose their pull specs.
568+
Dependencies []StepDependency `json:"dependencies,omitempty"`
566569
// OptionalOnSuccess defines if this step should be skipped as long
567570
// as all `pre` and `test` steps were successful and AllowSkipOnSuccess
568571
// flag is set to true in MultiStageTestConfiguration. This option is
@@ -590,6 +593,15 @@ type CredentialReference struct {
590593
MountPath string `json:"mount_path"`
591594
}
592595

596+
// StepDependency defines a dependency on an image and the environment variable
597+
// used to expose the image's pull spec to the step.
598+
type StepDependency struct {
599+
// Name is the tag or stream:tag that this dependency references
600+
Name string `json:"name"`
601+
// Env is the environment variable that the image's pull spec is exposed with
602+
Env string `json:"env"`
603+
}
604+
593605
// FromImageTag returns the internal name for the image tag that will be used
594606
// for this step, if one is configured.
595607
func (s *LiteralTestStep) FromImageTag() (PipelineImageStreamTagReference, bool) {

pkg/defaults/defaults.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,7 @@ func FromConfig(
224224
step = release.ImportReleaseStep(resolveConfig.Name, value, false, config.Resources, podClient, imageClient, saGetter, rbacClient, artifactDir, jobSpec)
225225
} else if testStep := rawStep.TestStepConfiguration; testStep != nil {
226226
if test := testStep.MultiStageTestConfigurationLiteral; test != nil {
227-
step = steps.MultiStageTestStep(*testStep, config, params, podClient, secretGetter, saGetter, rbacClient, artifactDir, jobSpec)
227+
step = steps.MultiStageTestStep(*testStep, config, params, podClient, secretGetter, saGetter, rbacClient, imageClient, artifactDir, jobSpec)
228228
if test.ClusterProfile != "" {
229229
step = steps.LeaseStep(leaseClient, test.ClusterProfile.LeaseType(), step, jobSpec.Namespace, namespaceClient)
230230
}

pkg/steps/multi_stage.go

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"path/filepath"
88
"strings"
99

10+
imageclientset "github.com/openshift/client-go/image/clientset/versioned/typed/image/v1"
1011
coreapi "k8s.io/api/core/v1"
1112
rbacapi "k8s.io/api/rbac/v1"
1213
"k8s.io/apimachinery/pkg/api/errors"
@@ -53,6 +54,7 @@ type multiStageTestStep struct {
5354
secretClient coreclientset.SecretsGetter
5455
saClient coreclientset.ServiceAccountsGetter
5556
rbacClient rbacclientset.RbacV1Interface
57+
isClient imageclientset.ImageStreamsGetter
5658
artifactDir string
5759
jobSpec *api.JobSpec
5860
pre, test, post []api.LiteralTestStep
@@ -68,10 +70,11 @@ func MultiStageTestStep(
6870
secretClient coreclientset.SecretsGetter,
6971
saClient coreclientset.ServiceAccountsGetter,
7072
rbacClient rbacclientset.RbacV1Interface,
73+
isClient imageclientset.ImageStreamsGetter,
7174
artifactDir string,
7275
jobSpec *api.JobSpec,
7376
) api.Step {
74-
return newMultiStageTestStep(testConfig, config, params, podClient, secretClient, saClient, rbacClient, artifactDir, jobSpec)
77+
return newMultiStageTestStep(testConfig, config, params, podClient, secretClient, saClient, rbacClient, isClient, artifactDir, jobSpec)
7578
}
7679

7780
func newMultiStageTestStep(
@@ -82,6 +85,7 @@ func newMultiStageTestStep(
8285
secretClient coreclientset.SecretsGetter,
8386
saClient coreclientset.ServiceAccountsGetter,
8487
rbacClient rbacclientset.RbacV1Interface,
88+
isClient imageclientset.ImageStreamsGetter,
8589
artifactDir string,
8690
jobSpec *api.JobSpec,
8791
) *multiStageTestStep {
@@ -99,6 +103,7 @@ func newMultiStageTestStep(
99103
secretClient: secretClient,
100104
saClient: saClient,
101105
rbacClient: rbacClient,
106+
isClient: isClient,
102107
artifactDir: artifactDir,
103108
jobSpec: jobSpec,
104109
pre: ms.Pre,
@@ -179,6 +184,13 @@ func (s *multiStageTestStep) Requires() (ret []api.StepLink) {
179184
if link, ok := step.FromImageTag(); ok {
180185
internalLinks[link] = struct{}{}
181186
}
187+
188+
for _, dependency := range step.Dependencies {
189+
imageStream, name := s.parts(dependency)
190+
if link, ok := utils.LinkForImage(imageStream, name); ok {
191+
ret = append(ret, link)
192+
}
193+
}
182194
}
183195
for link := range internalLinks {
184196
ret = append(ret, api.InternalImageLink(link))
@@ -197,6 +209,17 @@ func (s *multiStageTestStep) Requires() (ret []api.StepLink) {
197209
return
198210
}
199211

212+
// parts returns the imageStream and tag name from a user-provided
213+
// reference to an image in the test namespace
214+
func (s multiStageTestStep) parts(dependency api.StepDependency) (string, string) {
215+
if !strings.Contains(dependency.Name, ":") {
216+
return s.imageStreamFor(dependency.Name), dependency.Name
217+
} else {
218+
parts := strings.Split(dependency.Name, ":")
219+
return parts[0], parts[1]
220+
}
221+
}
222+
200223
func (s *multiStageTestStep) Creates() []api.StepLink { return nil }
201224
func (s *multiStageTestStep) Provides() api.ParameterMap {
202225
return nil
@@ -321,6 +344,21 @@ func (s *multiStageTestStep) runSteps(
321344
return utilerrors.NewAggregate(errs)
322345
}
323346

347+
// imageStreamFor guesses at the ImageStream that will hold a tag.
348+
// We use this to decipher the user's intent when they provide a
349+
// naked tag in configuration; we support such behavior in order to
350+
// allow users a simpler workflow for the most common cases, like
351+
// referring to `pipeline:src`. If they refer to an ambiguous image,
352+
// however, they will get bad behavior and will need to specify an
353+
// ImageStrem as well, for instance release-initial:installer.
354+
func (s *multiStageTestStep) imageStreamFor(image string) string {
355+
if s.config.IsPipelineImage(image) || s.config.BuildsImage(image) {
356+
return api.PipelineImageStream
357+
} else {
358+
return api.StableImageStream
359+
}
360+
}
361+
324362
func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []coreapi.EnvVar,
325363
hasPrevErrs bool) ([]coreapi.Pod, error) {
326364
var ret []coreapi.Pod
@@ -335,11 +373,7 @@ func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []cor
335373
if link, ok := step.FromImageTag(); ok {
336374
image = fmt.Sprintf("%s:%s", api.PipelineImageStream, link)
337375
} else {
338-
if s.config.IsPipelineImage(image) || s.config.BuildsImage(image) {
339-
image = fmt.Sprintf("%s:%s", api.PipelineImageStream, image)
340-
} else {
341-
image = fmt.Sprintf("%s:%s", api.StableImageStream, image)
342-
}
376+
image = fmt.Sprintf("%s:%s", s.imageStreamFor(image), image)
343377
}
344378
resources, err := resourcesFor(step.Resources)
345379
if err != nil {
@@ -365,6 +399,12 @@ func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []cor
365399
}...)
366400
container.Env = append(container.Env, env...)
367401
container.Env = append(container.Env, s.generateParams(step.Environment)...)
402+
depEnv, depErrs := s.envForDependencies(step)
403+
if len(depErrs) != 0 {
404+
errs = append(errs, depErrs...)
405+
continue
406+
}
407+
container.Env = append(container.Env, depEnv...)
368408
if owner := s.jobSpec.Owner(); owner != nil {
369409
pod.OwnerReferences = append(pod.OwnerReferences, *owner)
370410
}
@@ -381,6 +421,23 @@ func (s *multiStageTestStep) generatePods(steps []api.LiteralTestStep, env []cor
381421
return ret, utilerrors.NewAggregate(errs)
382422
}
383423

424+
func (s *multiStageTestStep) envForDependencies(step api.LiteralTestStep) ([]coreapi.EnvVar, []error) {
425+
var env []coreapi.EnvVar
426+
var errs []error
427+
for _, dependency := range step.Dependencies {
428+
imageStream, name := s.parts(dependency)
429+
ref, err := utils.ImageDigestFor(s.isClient, s.jobSpec.Namespace, imageStream, name)()
430+
if err != nil {
431+
errs = append(errs, fmt.Errorf("could not determine image pull spec for image %s on step %s", dependency.Name, step.As))
432+
continue
433+
}
434+
env = append(env, coreapi.EnvVar{
435+
Name: dependency.Env, Value: ref,
436+
})
437+
}
438+
return env, errs
439+
}
440+
384441
func addSecretWrapper(pod *coreapi.Pod) {
385442
volume := "secret-wrapper"
386443
dir := "/tmp/secret-wrapper"

pkg/steps/multi_stage_test.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,15 @@ package steps
22

33
import (
44
"context"
5-
"github.com/google/go-cmp/cmp"
65
"io/ioutil"
76
"os"
87
"path/filepath"
98
"reflect"
109
"testing"
1110
"time"
1211

12+
"github.com/google/go-cmp/cmp"
13+
1314
coreapi "k8s.io/api/core/v1"
1415
"k8s.io/apimachinery/pkg/api/equality"
1516
meta "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -34,7 +35,7 @@ func TestRequires(t *testing.T) {
3435
steps api.MultiStageTestConfigurationLiteral
3536
req []api.StepLink
3637
}{{
37-
name: "step has a cluster profile and requires a release image, should not have StableImagesLink",
38+
name: "step has a cluster profile and requires a release image, should not have ReleaseImagesLink",
3839
steps: api.MultiStageTestConfigurationLiteral{
3940
ClusterProfile: api.ClusterProfileAWS,
4041
Test: []api.LiteralTestStep{{From: "from-release"}},
@@ -44,7 +45,7 @@ func TestRequires(t *testing.T) {
4445
api.ImagesReadyLink(),
4546
},
4647
}, {
47-
name: "step needs release images, should have StableImagesLink",
48+
name: "step needs release images, should have ReleaseImagesLink",
4849
steps: api.MultiStageTestConfigurationLiteral{
4950
Test: []api.LiteralTestStep{{From: "from-release"}},
5051
},
@@ -75,7 +76,7 @@ func TestRequires(t *testing.T) {
7576
t.Run(tc.name, func(t *testing.T) {
7677
step := MultiStageTestStep(api.TestStepConfiguration{
7778
MultiStageTestConfigurationLiteral: &tc.steps,
78-
}, &tc.config, api.NewDeferredParameters(), nil, nil, nil, nil, "", nil)
79+
}, &tc.config, api.NewDeferredParameters(), nil, nil, nil, nil, nil, "", nil)
7980
ret := step.Requires()
8081
if len(ret) == len(tc.req) {
8182
matches := true
@@ -127,7 +128,7 @@ func TestGeneratePods(t *testing.T) {
127128
},
128129
}
129130
jobSpec.SetNamespace("namespace")
130-
step := newMultiStageTestStep(config.Tests[0], &config, nil, nil, nil, nil, nil, "artifact_dir", &jobSpec)
131+
step := newMultiStageTestStep(config.Tests[0], &config, nil, nil, nil, nil, nil, nil, "artifact_dir", &jobSpec)
131132
env := []coreapi.EnvVar{
132133
{Name: "RELEASE_IMAGE_INITIAL", Value: "release:initial"},
133134
{Name: "RELEASE_IMAGE_LATEST", Value: "release:latest"},
@@ -197,7 +198,7 @@ func TestGeneratePodsEnvironment(t *testing.T) {
197198
Test: test,
198199
Environment: tc.env,
199200
},
200-
}, &api.ReleaseBuildConfiguration{}, nil, nil, nil, nil, nil, "", &jobSpec)
201+
}, &api.ReleaseBuildConfiguration{}, nil, nil, nil, nil, nil, nil, "", &jobSpec)
201202
pods, err := step.(*multiStageTestStep).generatePods(test, nil, false)
202203
if err != nil {
203204
t.Fatal(err)
@@ -322,7 +323,7 @@ func TestRun(t *testing.T) {
322323
Post: []api.LiteralTestStep{{As: "post0"}, {As: "post1", OptionalOnSuccess: &yes}},
323324
AllowSkipOnSuccess: &yes,
324325
},
325-
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), "", &jobSpec)
326+
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), nil, "", &jobSpec)
326327
if err := step.Run(context.Background()); tc.failures == nil && err != nil {
327328
t.Error(err)
328329
return
@@ -378,7 +379,7 @@ func TestArtifacts(t *testing.T) {
378379
{As: "test1", ArtifactDir: "/path/to/artifacts"},
379380
},
380381
},
381-
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), tmp, &jobSpec)
382+
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), nil, tmp, &jobSpec)
382383
if err := step.Run(context.Background()); err != nil {
383384
t.Fatal(err)
384385
}
@@ -455,7 +456,7 @@ func TestJUnit(t *testing.T) {
455456
Test: []api.LiteralTestStep{{As: "test0"}, {As: "test1"}},
456457
Post: []api.LiteralTestStep{{As: "post0"}, {As: "post1"}},
457458
},
458-
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), "/dev/null", &jobSpec)
459+
}, &api.ReleaseBuildConfiguration{}, nil, &fakePodClient{NewPodClient(client, nil, nil)}, client, client, fakecs.RbacV1(), nil, "/dev/null", &jobSpec)
459460
if err := step.Run(context.Background()); tc.failures == nil && err != nil {
460461
t.Error(err)
461462
return

pkg/steps/utils/env.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,25 @@ func ReleaseNameFrom(envVar string) string {
154154
name, _ := imageFromEnv(api.ReleaseImageStream, envVar)
155155
return name
156156
}
157+
158+
// LinkForImage determines what dependent link is required
159+
// for the user's image dependency
160+
func LinkForImage(imageStream, tag string) (api.StepLink, bool) {
161+
switch {
162+
case imageStream == api.PipelineImageStream:
163+
// the user needs an image we're building
164+
return api.InternalImageLink(api.PipelineImageStreamTagReference(tag)), true
165+
case api.IsReleaseStream(imageStream):
166+
// the user needs a tag that's a component of some release;
167+
// we cant' rely on a specific tag, as they are implicit in
168+
// the import process and won't be present in the build graph,
169+
// so we wait for the whole import to succeed
170+
return api.ReleaseImagesLink(api.ReleaseNameFrom(imageStream)), true
171+
case api.IsReleasePayloadStream(imageStream):
172+
// the user needs a release payload
173+
return api.ReleasePayloadImageLink(tag), true
174+
default:
175+
// we have no idea what the user's configured
176+
return nil, false
177+
}
178+
}

0 commit comments

Comments
 (0)