From 464864960e0b6463051522c06e3a622e91f1d198 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 19 Jan 2015 22:31:44 -0500 Subject: [PATCH 01/18] UPSTREAM: Expose validation.ValidateLabels for reuse --- .../kubernetes/pkg/api/validation/validation.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go index 5e64bb80a0cd..80b059bce6bf 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation/validation.go @@ -378,7 +378,7 @@ func ValidatePod(pod *api.Pod) errs.ValidationErrorList { allErrs = append(allErrs, errs.NewFieldInvalid("namespace", pod.Namespace, "")) } allErrs = append(allErrs, ValidatePodSpec(&pod.Spec).Prefix("spec")...) - allErrs = append(allErrs, validateLabels(pod.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(pod.Labels, "labels")...) return allErrs } @@ -394,11 +394,11 @@ func ValidatePodSpec(spec *api.PodSpec) errs.ValidationErrorList { allErrs = append(allErrs, validateContainers(spec.Containers, allVolumes).Prefix("containers")...) allErrs = append(allErrs, validateRestartPolicy(&spec.RestartPolicy).Prefix("restartPolicy")...) allErrs = append(allErrs, validateDNSPolicy(&spec.DNSPolicy).Prefix("dnsPolicy")...) - allErrs = append(allErrs, validateLabels(spec.NodeSelector, "nodeSelector")...) + allErrs = append(allErrs, ValidateLabels(spec.NodeSelector, "nodeSelector")...) return allErrs } -func validateLabels(labels map[string]string, field string) errs.ValidationErrorList { +func ValidateLabels(labels map[string]string, field string) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} for k := range labels { if !util.IsQualifiedName(k) { @@ -458,9 +458,9 @@ func ValidateService(service *api.Service, lister ServiceLister, ctx api.Context } if service.Spec.Selector != nil { - allErrs = append(allErrs, validateLabels(service.Spec.Selector, "spec.selector")...) + allErrs = append(allErrs, ValidateLabels(service.Spec.Selector, "spec.selector")...) } - allErrs = append(allErrs, validateLabels(service.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(service.Labels, "labels")...) if service.Spec.CreateExternalLoadBalancer { services, err := lister.ListServices(ctx) @@ -496,7 +496,7 @@ func ValidateReplicationController(controller *api.ReplicationController) errs.V allErrs = append(allErrs, errs.NewFieldInvalid("namespace", controller.Namespace, "")) } allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...) - allErrs = append(allErrs, validateLabels(controller.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(controller.Labels, "labels")...) return allErrs } @@ -533,7 +533,7 @@ func ValidateReplicationControllerSpec(spec *api.ReplicationControllerSpec) errs // ValidatePodTemplateSpec validates the spec of a pod template func ValidatePodTemplateSpec(spec *api.PodTemplateSpec) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} - allErrs = append(allErrs, validateLabels(spec.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(spec.Labels, "labels")...) allErrs = append(allErrs, ValidatePodSpec(&spec.Spec).Prefix("spec")...) allErrs = append(allErrs, ValidateReadOnlyPersistentDisks(spec.Spec.Volumes).Prefix("spec.volumes")...) return allErrs @@ -577,7 +577,7 @@ func ValidateMinion(minion *api.Node) errs.ValidationErrorList { if len(minion.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", minion.Name)) } - allErrs = append(allErrs, validateLabels(minion.Labels, "labels")...) + allErrs = append(allErrs, ValidateLabels(minion.Labels, "labels")...) return allErrs } From 033bc8018f62781f721753d90966b5c1c438a6f0 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:23:48 -0500 Subject: [PATCH 02/18] UPSTREAM: Add RunUntil(stopCh) to reflector and poller to allow termination --- .../kubernetes/pkg/client/cache/poller.go | 24 ++++++++++++------- .../kubernetes/pkg/client/cache/reflector.go | 6 +++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/poller.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/poller.go index 70fec9361da4..deb708320de6 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/poller.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/poller.go @@ -57,14 +57,22 @@ func NewPoller(getFunc GetFunc, period time.Duration, store Store) *Poller { // Run begins polling. It starts a goroutine and returns immediately. func (p *Poller) Run() { - go util.Forever(func() { - e, err := p.getFunc() - if err != nil { - glog.Errorf("failed to list: %v", err) - return - } - p.sync(e) - }, p.period) + go util.Forever(p.run, p.period) +} + +// RunUntil begins polling. It starts a goroutine and returns immediately. +// It will stop when the stopCh is closed. +func (p *Poller) RunUntil(stopCh <-chan struct{}) { + go util.Until(p.run, p.period, stopCh) +} + +func (p *Poller) run() { + e, err := p.getFunc() + if err != nil { + glog.Errorf("failed to list: %v", err) + return + } + p.sync(e) } func (p *Poller) sync(e Enumerator) { diff --git a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/reflector.go b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/reflector.go index 56ecdecc50e7..68ae3f22b057 100644 --- a/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/reflector.go +++ b/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache/reflector.go @@ -72,6 +72,12 @@ func (r *Reflector) Run() { go util.Forever(func() { r.listAndWatch() }, r.period) } +// RunUntil starts a watch and handles watch events. Will restart the watch if it is closed. +// RunUntil starts a goroutine and returns immediately. It will exit when stopCh is closed. +func (r *Reflector) RunUntil(stopCh <-chan struct{}) { + go util.Until(func() { r.listAndWatch() }, r.period, stopCh) +} + func (r *Reflector) listAndWatch() { var resourceVersion string From 44dae5e72010eee64d410ee504050ba9f0690c64 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Mon, 19 Jan 2015 22:56:56 -0500 Subject: [PATCH 03/18] Define "to" and "dockerImageReference" as new fields on BuildOutput Instead of using ImageTag and Registry, which must be joined together, allow clients to provide one of two options. "to", which may be a reference to an ImageRepository, or "dockerImageReference", which is a Docker push spec (the value you would tag an image before docker pushing it to a registry). Support backwards compatibility for ImageTag and Registry. Fix versioning of "BUILD" output - the field must be to a locked API version so that an upgrade during a build doesn't break it. Add proper validations on Build and BuildConfig, so that names, namespaces, and labels validate according to Kubernetes rules, and also increase the validation around the two new fields. --- pkg/build/api/types.go | 19 +- pkg/build/api/v1beta1/conversion.go | 33 +++ pkg/build/api/v1beta1/types.go | 20 ++ pkg/build/api/validation/validation.go | 72 +++++-- pkg/build/api/validation/validation_test.go | 203 ++++++++++++++---- pkg/build/builder/cmd/builder.go | 20 +- pkg/build/builder/docker.go | 2 +- pkg/build/builder/sti.go | 2 +- pkg/build/builder/util.go | 9 +- pkg/build/builder/util_test.go | 16 +- pkg/build/controller/controller_test.go | 2 +- pkg/build/controller/strategy/custom.go | 6 +- pkg/build/controller/strategy/custom_test.go | 7 +- pkg/build/controller/strategy/docker.go | 7 +- pkg/build/controller/strategy/docker_test.go | 7 +- pkg/build/controller/strategy/sti.go | 6 +- pkg/build/controller/strategy/sti_test.go | 7 +- pkg/build/controller/strategy/util.go | 9 +- pkg/build/registry/build/rest_test.go | 8 +- pkg/build/registry/buildconfig/rest_test.go | 24 +-- pkg/build/registry/etcd/etcd_test.go | 4 +- pkg/build/util/generate_test.go | 3 +- pkg/cmd/cli/describe/describer.go | 4 +- pkg/image/api/helper.go | 13 +- pkg/image/api/helper_test.go | 5 +- test/integration/buildcfgclient_test.go | 4 +- test/integration/buildclient_test.go | 2 +- .../imagechange_buildtrigger_test.go | 4 +- test/integration/webhookgithub_test.go | 4 +- 29 files changed, 374 insertions(+), 148 deletions(-) diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index c6637ccf058c..0b521a35f452 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -206,11 +206,22 @@ type STIBuildStrategy struct { // BuildOutput is input to a build strategy and describes the Docker image that the strategy // should produce. type BuildOutput struct { - // ImageTag is the tag to give to the image resulting from the build. - ImageTag string `json:"imageTag,omitempty"` + // To defines an optional ImageRepository to push the output of this build to. The namespace + // may be empty, in which case the named ImageRepository will be retrieved from the namespace + // of the build. Kind must be set to 'ImageRepository' and is the only supported value. If set, + // this field takes priority over DockerImageReference. This value will be used to look up + // a Docker image repository to push to. + To *kapi.ObjectReference `json:"to,omitempty"` + + // Tag is the "version name" that will be associated with the output image. This + // field is only used if the To field is set, and is ignored when DockerImageReference is used. + // This value represents a consistent name for a set of related changes (v1, 5.x, 5.5, dev, stable) + // and defaults to the preferred tag for "To" if not specified. + Tag string `json:"tag,omitempty"` - // Registry is the Docker registry which should receive the resulting built image via push. - Registry string `json:"registry,omitempty"` + // DockerImageReference is the full name of an image ([registry/]name[:tag]), and will be the + // value sent to Docker push at the end of a build. + DockerImageReference string `json:"dockerImageReference,omitempty"` } // BuildConfigLabel is the key of a Build label whose value is the ID of a BuildConfig diff --git a/pkg/build/api/v1beta1/conversion.go b/pkg/build/api/v1beta1/conversion.go index b513d664aed3..fa08ed18cb6b 100644 --- a/pkg/build/api/v1beta1/conversion.go +++ b/pkg/build/api/v1beta1/conversion.go @@ -4,6 +4,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" newer "github.com/openshift/origin/pkg/build/api" + image "github.com/openshift/origin/pkg/image/api" ) func init() { @@ -26,5 +27,37 @@ func init() { } return nil }, + // Deprecate ImageTag and Registry, replace with To / Tag / DockerImageReference + func(in *newer.BuildOutput, out *BuildOutput, s conversion.Scope) error { + if err := s.Convert(&in.To, &out.To, 0); err != nil { + return err + } + out.Tag = in.Tag + if len(in.DockerImageReference) > 0 { + out.DockerImageReference = in.DockerImageReference + registry, namespace, name, tag, _ := image.SplitDockerPullSpec(in.DockerImageReference) + out.Registry = registry + out.ImageTag = image.JoinDockerPullSpec("", namespace, name, tag) + } + return nil + }, + func(in *BuildOutput, out *newer.BuildOutput, s conversion.Scope) error { + if err := s.Convert(&in.To, &out.To, 0); err != nil { + return err + } + out.Tag = in.Tag + if len(in.DockerImageReference) > 0 { + out.DockerImageReference = in.DockerImageReference + return nil + } + if len(in.ImageTag) != 0 { + _, namespace, name, tag, err := image.SplitDockerPullSpec(in.ImageTag) + if err != nil { + return err + } + out.DockerImageReference = image.JoinDockerPullSpec(in.Registry, namespace, name, tag) + } + return nil + }, ) } diff --git a/pkg/build/api/v1beta1/types.go b/pkg/build/api/v1beta1/types.go index 318bf3c793f9..00bf4847f2c7 100644 --- a/pkg/build/api/v1beta1/types.go +++ b/pkg/build/api/v1beta1/types.go @@ -201,10 +201,30 @@ type STIBuildStrategy struct { // BuildOutput is input to a build strategy and describes the Docker image that the strategy // should produce. type BuildOutput struct { + // To defines an optional ImageRepository to push the output of this build to. The namespace + // may be empty, in which case the ImageRepository will be looked for in the namespace of + // the build. Kind must be set to 'ImageRepository' and is the only supported value. If set, + // this field takes priority over DockerImageReference. This value will be used to look up + // a Docker image repository to push to. + To *kapi.ObjectReference `json:"to,omitempty"` + + // Tag is the "version" that will be set on the remote server when the image is created. This + // field is only used if the To field is set, and is ignored when DockerImageReference is used. + // This value represents a consistent name for a set of related changes (v1, 5.x, 5.5, dev, stable) + // and defaults to the preferred label for "To" if not specified. + Tag string `json:"tag,omitempty"` + + // DockerImageReference is the full name of an image ([registry/]name[:tag]), and will be the + // value sent to Docker push at the end of a build. If set, this field takes priority over + // ImageTag and Registry. + DockerImageReference string `json:"dockerImageReference,omitempty"` + // ImageTag is the tag to give to the image resulting from the build. + // DEPRECATED: use DockerImageReference ImageTag string `json:"imageTag,omitempty"` // Registry is the Docker registry which should receive the resulting built image via push. + // DEPRECATED: use DockerImageReference Registry string `json:"registry,omitempty"` } diff --git a/pkg/build/api/validation/validation.go b/pkg/build/api/validation/validation.go index bd4901c3dee0..3ec474115d61 100644 --- a/pkg/build/api/validation/validation.go +++ b/pkg/build/api/validation/validation.go @@ -4,8 +4,11 @@ import ( "net/url" errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" buildapi "github.com/openshift/origin/pkg/build/api" + image "github.com/openshift/origin/pkg/image/api" ) // ValidateBuild tests required fields for a Build. @@ -13,7 +16,15 @@ func ValidateBuild(build *buildapi.Build) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} if len(build.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", build.Name)) + } else if !util.IsDNSSubdomain(build.Name) { + allErrs = append(allErrs, errs.NewFieldInvalid("name", build.Name, "name must be a valid subdomain")) } + if len(build.Namespace) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("namespace", build.Namespace)) + } else if !util.IsDNSSubdomain(build.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("namespace", build.Namespace, "namespace must be a valid subdomain")) + } + allErrs = append(allErrs, validation.ValidateLabels(build.Labels, "labels")...) allErrs = append(allErrs, validateBuildParameters(&build.Parameters).Prefix("parameters")...) return allErrs } @@ -23,11 +34,20 @@ func ValidateBuildConfig(config *buildapi.BuildConfig) errs.ValidationErrorList allErrs := errs.ValidationErrorList{} if len(config.Name) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("name", config.Name)) + } else if !util.IsDNSSubdomain(config.Name) { + allErrs = append(allErrs, errs.NewFieldInvalid("name", config.Name, "name must be a valid subdomain")) + } + if len(config.Namespace) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("namespace", config.Namespace)) + } else if !util.IsDNSSubdomain(config.Namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("namespace", config.Namespace, "namespace must be a valid subdomain")) } + allErrs = append(allErrs, validation.ValidateLabels(config.Labels, "labels")...) for i := range config.Triggers { allErrs = append(allErrs, validateTrigger(&config.Triggers[i]).PrefixIndex(i).Prefix("triggers")...) } allErrs = append(allErrs, validateBuildParameters(&config.Parameters).Prefix("parameters")...) + allErrs = append(allErrs, validateBuildConfigOutput(&config.Parameters.Output).Prefix("parameters.output")...) return allErrs } @@ -44,10 +64,7 @@ func validateBuildParameters(params *buildapi.BuildParameters) errs.ValidationEr } } - if !isCustomBuild || (isCustomBuild && len(params.Output.ImageTag) != 0) { - allErrs = append(allErrs, validateOutput(¶ms.Output).Prefix("output")...) - } - + allErrs = append(allErrs, validateOutput(¶ms.Output).Prefix("output")...) allErrs = append(allErrs, validateStrategy(¶ms.Strategy).Prefix("strategy")...) return allErrs @@ -85,6 +102,45 @@ func validateRevision(revision *buildapi.SourceRevision) errs.ValidationErrorLis return allErrs } +func validateOutput(output *buildapi.BuildOutput) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + + // TODO: make part of a generic ValidateObjectReference method upstream. + if output.To != nil { + kind, name, namespace := output.To.Kind, output.To.Name, output.To.Namespace + if len(kind) == 0 { + kind = "ImageRepository" + output.To.Kind = kind + } + if kind != "ImageRepository" { + allErrs = append(allErrs, errs.NewFieldInvalid("to.kind", kind, "the target of build output must be 'ImageRepository'")) + } + if len(name) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("to.name", name)) + } else if !util.IsDNSSubdomain(name) { + allErrs = append(allErrs, errs.NewFieldInvalid("to.name", name, "name must be a valid subdomain")) + } + if len(namespace) != 0 && !util.IsDNSSubdomain(namespace) { + allErrs = append(allErrs, errs.NewFieldInvalid("to.namespace", namespace, "namespace must be a valid subdomain")) + } + } + + if len(output.DockerImageReference) != 0 { + if _, _, _, _, err := image.SplitDockerPullSpec(output.DockerImageReference); err != nil { + allErrs = append(allErrs, errs.NewFieldInvalid("dockerImageReference", output.DockerImageReference, err.Error())) + } + } + return allErrs +} + +func validateBuildConfigOutput(output *buildapi.BuildOutput) errs.ValidationErrorList { + allErrs := errs.ValidationErrorList{} + if len(output.DockerImageReference) != 0 && output.To != nil { + allErrs = append(allErrs, errs.NewFieldInvalid("dockerImageReference", output.DockerImageReference, "only one of 'dockerImageReference' and 'to' may be set")) + } + return allErrs +} + func validateStrategy(strategy *buildapi.BuildStrategy) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} @@ -128,14 +184,6 @@ func validateSTIStrategy(strategy *buildapi.STIBuildStrategy) errs.ValidationErr return allErrs } -func validateOutput(output *buildapi.BuildOutput) errs.ValidationErrorList { - allErrs := errs.ValidationErrorList{} - if len(output.ImageTag) == 0 { - allErrs = append(allErrs, errs.NewFieldRequired("imageTag", output.ImageTag)) - } - return allErrs -} - func validateTrigger(trigger *buildapi.BuildTriggerPolicy) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} if len(trigger.Type) == 0 { diff --git a/pkg/build/api/validation/validation_test.go b/pkg/build/api/validation/validation_test.go index 7bc991685d29..4827db06db41 100644 --- a/pkg/build/api/validation/validation_test.go +++ b/pkg/build/api/validation/validation_test.go @@ -11,7 +11,7 @@ import ( func TestBuildValdationSuccess(t *testing.T) { build := &buildapi.Build{ - ObjectMeta: kapi.ObjectMeta{Name: "buildID"}, + ObjectMeta: kapi.ObjectMeta{Name: "buildid", Namespace: "default"}, Parameters: buildapi.BuildParameters{ Source: buildapi.BuildSource{ Type: buildapi.BuildSourceGit, @@ -26,7 +26,7 @@ func TestBuildValdationSuccess(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/data", + DockerImageReference: "repository/data", }, }, Status: buildapi.BuildStatusNew, @@ -38,7 +38,7 @@ func TestBuildValdationSuccess(t *testing.T) { func TestBuildValidationFailure(t *testing.T) { build := &buildapi.Build{ - ObjectMeta: kapi.ObjectMeta{Name: ""}, + ObjectMeta: kapi.ObjectMeta{Name: "", Namespace: ""}, Parameters: buildapi.BuildParameters{ Source: buildapi.BuildSource{ Type: buildapi.BuildSourceGit, @@ -53,19 +53,19 @@ func TestBuildValidationFailure(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/data", + DockerImageReference: "repository/data", }, }, Status: buildapi.BuildStatusNew, } - if result := ValidateBuild(build); len(result) != 1 { + if result := ValidateBuild(build); len(result) != 2 { t.Errorf("Unexpected validation result: %v", result) } } func TestBuildConfigValidationSuccess(t *testing.T) { buildConfig := &buildapi.BuildConfig{ - ObjectMeta: kapi.ObjectMeta{Name: "configId"}, + ObjectMeta: kapi.ObjectMeta{Name: "config-id", Namespace: "namespace"}, Parameters: buildapi.BuildParameters{ Source: buildapi.BuildSource{ Type: buildapi.BuildSourceGit, @@ -79,9 +79,7 @@ func TestBuildConfigValidationSuccess(t *testing.T) { ContextDir: "context", }, }, - Output: buildapi.BuildOutput{ - ImageTag: "repository/data", - }, + Output: buildapi.BuildOutput{}, }, } if result := ValidateBuildConfig(buildConfig); len(result) > 0 { @@ -91,7 +89,7 @@ func TestBuildConfigValidationSuccess(t *testing.T) { func TestBuildConfigValidationFailure(t *testing.T) { buildConfig := &buildapi.BuildConfig{ - ObjectMeta: kapi.ObjectMeta{Name: ""}, + ObjectMeta: kapi.ObjectMeta{Name: "", Namespace: "foo"}, Parameters: buildapi.BuildParameters{ Source: buildapi.BuildSource{ Type: buildapi.BuildSourceGit, @@ -106,7 +104,7 @@ func TestBuildConfigValidationFailure(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/data", + DockerImageReference: "repository/data", }, }, } @@ -115,6 +113,35 @@ func TestBuildConfigValidationFailure(t *testing.T) { } } +func TestBuildConfigValidationOutputFailure(t *testing.T) { + buildConfig := &buildapi.BuildConfig{ + ObjectMeta: kapi.ObjectMeta{Name: ""}, + Parameters: buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + DockerImageReference: "repository/data", + To: &kapi.ObjectReference{ + Name: "other", + }, + }, + }, + } + if result := ValidateBuildConfig(buildConfig); len(result) != 3 { + t.Errorf("Unexpected validation result %v", result) + } +} + func TestValidateSource(t *testing.T) { errorCases := map[string]*buildapi.BuildSource{ string(errs.ValidationErrorTypeRequired) + "git.uri": { @@ -144,52 +171,150 @@ func TestValidateSource(t *testing.T) { } func TestValidateBuildParameters(t *testing.T) { - errorCases := map[string]*buildapi.BuildParameters{ - string(errs.ValidationErrorTypeRequired) + "output.imageTag": { - Source: buildapi.BuildSource{ - Type: buildapi.BuildSourceGit, - Git: &buildapi.GitBuildSource{ - URI: "http://github.com/my/repository", + errorCases := []struct { + err string + *buildapi.BuildParameters + }{ + { + string(errs.ValidationErrorTypeInvalid) + "output.dockerImageReference", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + DockerImageReference: "some/long/value/with/no/meaning", }, }, - Strategy: buildapi.BuildStrategy{ - Type: buildapi.DockerBuildStrategyType, - DockerStrategy: &buildapi.DockerBuildStrategy{ - ContextDir: "context", + }, + { + string(errs.ValidationErrorTypeInvalid) + "output.to.kind", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Kind: "Foo", + Name: "test", + }, }, }, - Output: buildapi.BuildOutput{ - ImageTag: "", + }, + { + string(errs.ValidationErrorTypeRequired) + "output.to.name", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + To: &kapi.ObjectReference{}, + }, }, }, - string(errs.ValidationErrorTypeRequired) + "strategy.stiStrategy.image": { - Source: buildapi.BuildSource{ - Type: buildapi.BuildSourceGit, - Git: &buildapi.GitBuildSource{ - URI: "http://github.com/my/repository", + { + string(errs.ValidationErrorTypeInvalid) + "output.to.name", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "not_a_valid_subdomain", + Namespace: "subdomain", + }, }, }, - Output: buildapi.BuildOutput{ - ImageTag: "repository/data", + }, + { + string(errs.ValidationErrorTypeInvalid) + "output.to.namespace", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.DockerBuildStrategyType, + DockerStrategy: &buildapi.DockerBuildStrategy{ + ContextDir: "context", + }, + }, + Output: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "test", + Namespace: "not_a_valid_subdomain", + }, + }, }, - Strategy: buildapi.BuildStrategy{ - Type: buildapi.STIBuildStrategyType, - STIStrategy: &buildapi.STIBuildStrategy{ - Image: "", + }, + { + string(errs.ValidationErrorTypeRequired) + "strategy.stiStrategy.image", + &buildapi.BuildParameters{ + Source: buildapi.BuildSource{ + Type: buildapi.BuildSourceGit, + Git: &buildapi.GitBuildSource{ + URI: "http://github.com/my/repository", + }, + }, + Output: buildapi.BuildOutput{ + DockerImageReference: "repository/data", + }, + Strategy: buildapi.BuildStrategy{ + Type: buildapi.STIBuildStrategyType, + STIStrategy: &buildapi.STIBuildStrategy{ + Image: "", + }, }, }, }, } - for desc, config := range errorCases { - errors := validateBuildParameters(config) + for _, config := range errorCases { + errors := validateBuildParameters(config.BuildParameters) if len(errors) != 1 { - t.Errorf("%s: Unexpected validation result: %v", desc, errors) + t.Errorf("%s: Unexpected validation result: %v", config.err, errors) } err := errors[0].(*errs.ValidationError) errDesc := string(err.Type) + err.Field - if desc != errDesc { - t.Errorf("Unexpected validation result for %s: expected %s, got %s", err.Field, desc, errDesc) + if config.err != errDesc { + t.Errorf("Unexpected validation result for %s: expected %s, got %s", err.Field, config.err, errDesc) } } } diff --git a/pkg/build/builder/cmd/builder.go b/pkg/build/builder/cmd/builder.go index 075625c51ff7..c62438e994b8 100644 --- a/pkg/build/builder/cmd/builder.go +++ b/pkg/build/builder/cmd/builder.go @@ -1,15 +1,16 @@ package cmd import ( - "encoding/json" "log" "os" "github.com/fsouza/go-dockerclient" + "github.com/openshift/origin/pkg/api/latest" "github.com/openshift/origin/pkg/build/api" bld "github.com/openshift/origin/pkg/build/builder" "github.com/openshift/origin/pkg/build/builder/cmd/dockercfg" dockerutil "github.com/openshift/origin/pkg/cmd/util/docker" + image "github.com/openshift/origin/pkg/image/api" ) const DefaultDockerEndpoint = "unix:///var/run/docker.sock" @@ -32,11 +33,22 @@ func run(builderFactory factoryFunc) { } buildStr := os.Getenv("BUILD") build := api.Build{} - err = json.Unmarshal([]byte(buildStr), &build) - if err != nil { + if err := latest.Codec.DecodeInto([]byte(buildStr), &build); err != nil { log.Fatalf("Unable to parse build: %v", err) } - authcfg, authPresent := dockercfg.NewHelper().GetDockerAuth(build.Parameters.Output.Registry) + + var ( + authcfg docker.AuthConfiguration + authPresent bool + ) + if len(build.Parameters.Output.DockerImageReference) != 0 { + registry, _, _, _, err := image.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference) + if err != nil { + log.Fatalf("Output does not have a valid Docker image reference: %v", err) + } + authcfg, authPresent = dockercfg.NewHelper().GetDockerAuth(registry) + } + b := builderFactory(client, endpoint, authcfg, authPresent, &build) if err = b.Build(); err != nil { log.Fatalf("Build error: %v", err) diff --git a/pkg/build/builder/docker.go b/pkg/build/builder/docker.go index 21eb3c4b4c32..675760060ca2 100644 --- a/pkg/build/builder/docker.go +++ b/pkg/build/builder/docker.go @@ -65,7 +65,7 @@ func (d *DockerBuilder) Build() error { return err } defer removeImage(d.dockerClient, imageTag(d.build)) - if d.build.Parameters.Output.Registry != "" || d.authPresent { + if len(d.build.Parameters.Output.DockerImageReference) != 0 { return pushImage(d.dockerClient, imageTag(d.build), d.auth) } return nil diff --git a/pkg/build/builder/sti.go b/pkg/build/builder/sti.go index f37b8a17015d..04d4ccbf62ef 100644 --- a/pkg/build/builder/sti.go +++ b/pkg/build/builder/sti.go @@ -52,7 +52,7 @@ func (s *STIBuilder) Build() error { if _, err = builder.Build(); err != nil { return err } - if s.build.Parameters.Output.Registry != "" || s.authPresent { + if len(s.build.Parameters.Output.DockerImageReference) != 0 { return pushImage(s.dockerClient, imageTag(s.build), s.auth) } return nil diff --git a/pkg/build/builder/util.go b/pkg/build/builder/util.go index ee983a16370f..3d821d952351 100644 --- a/pkg/build/builder/util.go +++ b/pkg/build/builder/util.go @@ -1,9 +1,6 @@ package builder import ( - "fmt" - "strings" - "github.com/openshift/origin/pkg/build/api" ) @@ -14,11 +11,7 @@ type Builder interface { // imageTag returns the tag to be used for the build. If a registry has been // specified, it will prepend the registry to the name func imageTag(build *api.Build) string { - tag := build.Parameters.Output.ImageTag - if !strings.HasPrefix(tag, build.Parameters.Output.Registry) { - tag = fmt.Sprintf("%s/%s", build.Parameters.Output.Registry, tag) - } - return tag + return build.Parameters.Output.DockerImageReference } // getBuildEnvVars returns a map with the environment variables that should be added diff --git a/pkg/build/builder/util_test.go b/pkg/build/builder/util_test.go index a0da9b0c90de..eed2f836f9d4 100644 --- a/pkg/build/builder/util_test.go +++ b/pkg/build/builder/util_test.go @@ -17,7 +17,7 @@ func TestImageTag(t *testing.T) { build: api.Build{ Parameters: api.BuildParameters{ Output: api.BuildOutput{ - ImageTag: "test/tag", + DockerImageReference: "test/tag", }, }, }, @@ -27,19 +27,7 @@ func TestImageTag(t *testing.T) { build: api.Build{ Parameters: api.BuildParameters{ Output: api.BuildOutput{ - ImageTag: "test/tag", - Registry: "registry-server.test:5000", - }, - }, - }, - expected: "registry-server.test:5000/test/tag", - }, - { - build: api.Build{ - Parameters: api.BuildParameters{ - Output: api.BuildOutput{ - ImageTag: "registry-server.test:5000/test/tag", - Registry: "registry-server.test:5000", + DockerImageReference: "registry-server.test:5000/test/tag", }, }, }, diff --git a/pkg/build/controller/controller_test.go b/pkg/build/controller/controller_test.go index ffe89eadd046..0faea2f41c3c 100644 --- a/pkg/build/controller/controller_test.go +++ b/pkg/build/controller/controller_test.go @@ -87,7 +87,7 @@ func mockBuildAndController(status buildapi.BuildStatus) (build *buildapi.Build, }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/dataBuild", }, }, Status: status, diff --git a/pkg/build/controller/strategy/custom.go b/pkg/build/controller/strategy/custom.go index 1e96f94aeb45..fd687e27a7fe 100644 --- a/pkg/build/controller/strategy/custom.go +++ b/pkg/build/controller/strategy/custom.go @@ -1,12 +1,12 @@ package strategy import ( - "encoding/json" "errors" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/golang/glog" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -17,14 +17,14 @@ type CustomBuildStrategy struct { // CreateBuildPod creates the pod to be used for the Custom build func (bs *CustomBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - buildJSON, err := json.Marshal(build) + data, err := v1beta1.Codec.Encode(build) if err != nil { return nil, err } strategy := build.Parameters.Strategy.CustomStrategy containerEnv := []kapi.EnvVar{ - {Name: "BUILD", Value: string(buildJSON)}, + {Name: "BUILD", Value: string(data)}, {Name: "SOURCE_REPOSITORY", Value: build.Parameters.Source.Git.URI}, } diff --git a/pkg/build/controller/strategy/custom_test.go b/pkg/build/controller/strategy/custom_test.go index 53590e6ae3da..f30ffae1cf3f 100644 --- a/pkg/build/controller/strategy/custom_test.go +++ b/pkg/build/controller/strategy/custom_test.go @@ -1,11 +1,11 @@ package strategy import ( - "encoding/json" "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -41,7 +41,7 @@ func TestCustomCreateBuildPod(t *testing.T) { if actual.Spec.RestartPolicy.Never == nil { t.Errorf("Expected never, got %#v", actual.Spec.RestartPolicy) } - buildJSON, _ := json.Marshal(expected) + buildJSON, _ := v1beta1.Codec.Encode(expected) errorCases := map[int][]string{ 0: {"BUILD", string(buildJSON)}, } @@ -94,8 +94,7 @@ func mockCustomBuild() *buildapi.Build { }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/customBuild", - Registry: "docker-registry", + DockerImageReference: "docker-registry/repository/customBuild", }, }, Status: buildapi.BuildStatusNew, diff --git a/pkg/build/controller/strategy/docker.go b/pkg/build/controller/strategy/docker.go index 78c05e945ed8..974eedb15f6f 100644 --- a/pkg/build/controller/strategy/docker.go +++ b/pkg/build/controller/strategy/docker.go @@ -1,10 +1,9 @@ package strategy import ( - "encoding/json" - kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -17,7 +16,7 @@ type DockerBuildStrategy struct { // CreateBuildPod creates the pod to be used for the Docker build // TODO: Make the Pod definition configurable func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - buildJSON, err := json.Marshal(build) + data, err := v1beta1.Codec.Encode(build) if err != nil { return nil, err } @@ -32,7 +31,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, Name: "docker-build", Image: bs.Image, Env: []kapi.EnvVar{ - {Name: "BUILD", Value: string(buildJSON)}, + {Name: "BUILD", Value: string(data)}, }, // TODO: run unprivileged https://github.com/openshift/origin/issues/662 Privileged: true, diff --git a/pkg/build/controller/strategy/docker_test.go b/pkg/build/controller/strategy/docker_test.go index 314b57a5348e..8378c7a690d2 100644 --- a/pkg/build/controller/strategy/docker_test.go +++ b/pkg/build/controller/strategy/docker_test.go @@ -1,11 +1,11 @@ package strategy import ( - "encoding/json" "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -38,7 +38,7 @@ func TestDockerCreateBuildPod(t *testing.T) { if len(container.Env) != 1 { t.Fatalf("Expected 1 elements in Env table, got %d", len(container.Env)) } - buildJSON, _ := json.Marshal(expected) + buildJSON, _ := v1beta1.Codec.Encode(expected) errorCases := map[int][]string{ 0: {"BUILD", string(buildJSON)}, } @@ -71,8 +71,7 @@ func mockDockerBuild() *buildapi.Build { DockerStrategy: &buildapi.DockerBuildStrategy{ContextDir: "my/test/dir"}, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/dockerBuild", - Registry: "docker-registry", + DockerImageReference: "docker-registry/repository/dockerBuild", }, }, Status: buildapi.BuildStatusNew, diff --git a/pkg/build/controller/strategy/sti.go b/pkg/build/controller/strategy/sti.go index b22c04fc9bd7..e5731c037423 100644 --- a/pkg/build/controller/strategy/sti.go +++ b/pkg/build/controller/strategy/sti.go @@ -1,11 +1,11 @@ package strategy import ( - "encoding/json" "io/ioutil" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -31,7 +31,7 @@ var STITempDirectoryCreator = &tempDirectoryCreator{} // CreateBuildPod creates a pod that will execute the STI build // TODO: Make the Pod definition configurable func (bs *STIBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - buildJSON, err := json.Marshal(build) + data, err := v1beta1.Codec.Encode(build) if err != nil { return nil, err } @@ -45,7 +45,7 @@ func (bs *STIBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, er Name: "sti-build", Image: bs.Image, Env: []kapi.EnvVar{ - {Name: "BUILD", Value: string(buildJSON)}, + {Name: "BUILD", Value: string(data)}, }, // TODO: run unprivileged https://github.com/openshift/origin/issues/662 Privileged: true, diff --git a/pkg/build/controller/strategy/sti_test.go b/pkg/build/controller/strategy/sti_test.go index 9514f52f7ed3..a00560509f59 100644 --- a/pkg/build/controller/strategy/sti_test.go +++ b/pkg/build/controller/strategy/sti_test.go @@ -1,11 +1,11 @@ package strategy import ( - "encoding/json" "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -44,7 +44,7 @@ func TestSTICreateBuildPod(t *testing.T) { if len(container.Env) != 1 { t.Fatalf("Expected 1 elements in Env table, got %d", len(container.Env)) } - buildJSON, _ := json.Marshal(expected) + buildJSON, _ := v1beta1.Codec.Encode(expected) errorCases := map[int][]string{ 0: {"BUILD", string(buildJSON)}, } @@ -80,8 +80,7 @@ func mockSTIBuild() *buildapi.Build { }, }, Output: buildapi.BuildOutput{ - ImageTag: "repository/stiBuild", - Registry: "docker-registry", + DockerImageReference: "docker-registry/repository/stiBuild", }, }, Status: buildapi.BuildStatusNew, diff --git a/pkg/build/controller/strategy/util.go b/pkg/build/controller/strategy/util.go index 7dff7fbe884e..ae875550d124 100644 --- a/pkg/build/controller/strategy/util.go +++ b/pkg/build/controller/strategy/util.go @@ -6,6 +6,7 @@ import ( kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" buildapi "github.com/openshift/origin/pkg/build/api" + image "github.com/openshift/origin/pkg/image/api" ) // dockerSocketPath is the default path for the Docker socket inside the builder @@ -75,8 +76,12 @@ func setupBuildEnv(build *buildapi.Build, pod *kapi.Pod) { default: // Do nothing for unknown source types } - vars = append(vars, kapi.EnvVar{"OUTPUT_IMAGE", build.Parameters.Output.ImageTag}) - vars = append(vars, kapi.EnvVar{"OUTPUT_REGISTRY", build.Parameters.Output.Registry}) + + if registry, namespace, name, tag, err := image.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference); err == nil { + outputImage := image.JoinDockerPullSpec("", namespace, name, tag) + vars = append(vars, kapi.EnvVar{"OUTPUT_IMAGE", outputImage}) + vars = append(vars, kapi.EnvVar{"OUTPUT_REGISTRY", registry}) + } if len(pod.Spec.Containers) > 0 { pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, vars...) diff --git a/pkg/build/registry/build/rest_test.go b/pkg/build/registry/build/rest_test.go index eae4d662a8d6..4a317ae5b6c7 100644 --- a/pkg/build/registry/build/rest_test.go +++ b/pkg/build/registry/build/rest_test.go @@ -342,7 +342,7 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/data-build", }, }, }, @@ -365,10 +365,10 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { func mockBuild() *api.Build { return &api.Build{ ObjectMeta: kapi.ObjectMeta{ - Name: "dataBuild", + Name: "data-build", Namespace: kapi.NamespaceDefault, Labels: map[string]string{ - "name": "dataBuild", + "name": "data-build", }, }, Parameters: api.BuildParameters{ @@ -385,7 +385,7 @@ func mockBuild() *api.Build { }, }, Output: api.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/data-build", }, }, Status: api.BuildStatusPending, diff --git a/pkg/build/registry/buildconfig/rest_test.go b/pkg/build/registry/buildconfig/rest_test.go index 2dda9e3bcad7..3078d3bf84a1 100644 --- a/pkg/build/registry/buildconfig/rest_test.go +++ b/pkg/build/registry/buildconfig/rest_test.go @@ -254,10 +254,10 @@ func TestCreateBuildConfig(t *testing.T) { func mockBuildConfig() *api.BuildConfig { return &api.BuildConfig{ ObjectMeta: kapi.ObjectMeta{ - Name: "dataBuild", + Name: "data-build", Namespace: kapi.NamespaceDefault, Labels: map[string]string{ - "name": "dataBuild", + "name": "data-build", }, }, Parameters: api.BuildParameters{ @@ -274,7 +274,7 @@ func mockBuildConfig() *api.BuildConfig { }, }, Output: api.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/data-build", }, }, } @@ -350,11 +350,11 @@ func TestBuildConfigRESTValidatesCreate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "data/image", + DockerImageReference: "data/image", }, }, }, - "blank ImageTag": { + "blank DockerImageReference": { ObjectMeta: kapi.ObjectMeta{Name: "abc"}, Parameters: api.BuildParameters{ Source: api.BuildSource{ @@ -364,7 +364,7 @@ func TestBuildConfigRESTValidatesCreate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "", + DockerImageReference: "", }, }, }, @@ -384,7 +384,7 @@ func TestBuildConfigRESTValidatesCreate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "data/image", + DockerImageReference: "data/image", }, }, }, @@ -414,7 +414,7 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "data/image", + DockerImageReference: "data/image", }, }, }, @@ -434,11 +434,11 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "data/image", + DockerImageReference: "data/image", }, }, }, - "blank ImageTag": { + "blank DockerImageReference": { ObjectMeta: kapi.ObjectMeta{Name: "abc"}, Parameters: api.BuildParameters{ Source: api.BuildSource{ @@ -448,7 +448,7 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "", + DockerImageReference: "", }, }, }, @@ -468,7 +468,7 @@ func TestBuildRESTValidatesUpdate(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "data/image", + DockerImageReference: "data/image", }, }, }, diff --git a/pkg/build/registry/etcd/etcd_test.go b/pkg/build/registry/etcd/etcd_test.go index 240e57694cad..065811639f66 100644 --- a/pkg/build/registry/etcd/etcd_test.go +++ b/pkg/build/registry/etcd/etcd_test.go @@ -113,7 +113,7 @@ func TestEtcdCreateBuild(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/dataBuild", }, }, Status: api.BuildStatusPending, @@ -335,7 +335,7 @@ func TestEtcdCreateBuildConfig(t *testing.T) { }, }, Output: api.BuildOutput{ - ImageTag: "repository/dataBuild", + DockerImageReference: "repository/dataBuild", }, }, }) diff --git a/pkg/build/util/generate_test.go b/pkg/build/util/generate_test.go index 8bc4604f81da..10fea89fdcd8 100644 --- a/pkg/build/util/generate_test.go +++ b/pkg/build/util/generate_test.go @@ -330,8 +330,7 @@ func mockCustomStrategy() api.BuildStrategy { func mockOutput() api.BuildOutput { return api.BuildOutput{ - Registry: "http://localhost:5000", - ImageTag: "test/image-tag", + DockerImageReference: "http://localhost:5000/test/image-tag", } } diff --git a/pkg/cmd/cli/describe/describer.go b/pkg/cmd/cli/describe/describer.go index 18b005878d83..12d4279e955e 100644 --- a/pkg/cmd/cli/describe/describer.go +++ b/pkg/cmd/cli/describe/describer.go @@ -86,8 +86,8 @@ func (d *BuildDescriber) DescribeParameters(p buildapi.BuildParameters, out *tab formatString(out, "Ref", p.Source.Git.Ref) } } - formatString(out, "Output Image", p.Output.ImageTag) - formatString(out, "Output Registry", p.Output.Registry) + formatString(out, "Output To", p.Output.To) + formatString(out, "Output Spec", p.Output.DockerImageReference) if p.Revision != nil && p.Revision.Type == buildapi.BuildSourceGit && p.Revision.Git != nil { formatString(out, "Git Commit", p.Revision.Git.Commit) d.DescribeUser(out, "Revision Author", p.Revision.Git.Author) diff --git a/pkg/image/api/helper.go b/pkg/image/api/helper.go index 057bc31b20c6..9f78f789cf06 100644 --- a/pkg/image/api/helper.go +++ b/pkg/image/api/helper.go @@ -7,8 +7,8 @@ import ( "github.com/fsouza/go-dockerclient" ) -// dockerDefaultNamespace is the value for namespace when a single segment name is provided. -const dockerDefaultNamespace = "library" +// DockerDefaultNamespace is the value for namespace when a single segment name is provided. +const DockerDefaultNamespace = "library" // SplitDockerPullSpec breaks a Docker pull specification into its components, or returns // an error if those components are not valid. Attempts to match as closely as possible the @@ -18,9 +18,6 @@ func SplitDockerPullSpec(spec string) (registry, namespace, name, tag string, er if err != nil { return } - if len(namespace) == 0 { - namespace = dockerDefaultNamespace - } return } @@ -51,12 +48,12 @@ func SplitOpenShiftPullSpec(spec string) (registry, namespace, name, tag string, // string. Attempts to match as closely as possible the Docker spec up to 1.3. Future API // revisions may change the pull syntax. func JoinDockerPullSpec(registry, namespace, name, tag string) string { - if len(namespace) == 0 { - namespace = dockerDefaultNamespace - } if len(tag) != 0 { tag = ":" + tag } + if len(namespace) == 0 { + return fmt.Sprintf("%s%s", name, tag) + } if len(registry) == 0 { return fmt.Sprintf("%s/%s%s", namespace, name, tag) } diff --git a/pkg/image/api/helper_test.go b/pkg/image/api/helper_test.go index 083a930603b4..4818cdfe7cba 100644 --- a/pkg/image/api/helper_test.go +++ b/pkg/image/api/helper_test.go @@ -11,9 +11,8 @@ func TestSplitDockerPullSpec(t *testing.T) { Err bool }{ { - From: "foo", - Namespace: "library", - Name: "foo", + From: "foo", + Name: "foo", }, { From: "bar/foo", diff --git a/test/integration/buildcfgclient_test.go b/test/integration/buildcfgclient_test.go index c44da1fa97ba..02a62bb3dc8c 100644 --- a/test/integration/buildcfgclient_test.go +++ b/test/integration/buildcfgclient_test.go @@ -135,7 +135,7 @@ func mockBuildConfig() *buildapi.BuildConfig { }, }, Output: buildapi.BuildOutput{ - ImageTag: "namespace/builtimage", + DockerImageReference: "namespace/builtimage", }, }, } @@ -175,7 +175,7 @@ func TestBuildConfigClient(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "namespace/builtimage", + DockerImageReference: "namespace/builtimage", }, }, } diff --git a/test/integration/buildclient_test.go b/test/integration/buildclient_test.go index aef04111be3b..6be1e85755ec 100644 --- a/test/integration/buildclient_test.go +++ b/test/integration/buildclient_test.go @@ -127,7 +127,7 @@ func mockBuild() *buildapi.Build { }, }, Output: buildapi.BuildOutput{ - ImageTag: "namespace/builtimage", + DockerImageReference: "namespace/builtimage", }, }, } diff --git a/test/integration/imagechange_buildtrigger_test.go b/test/integration/imagechange_buildtrigger_test.go index 5a01d5d4c641..361a422b2478 100644 --- a/test/integration/imagechange_buildtrigger_test.go +++ b/test/integration/imagechange_buildtrigger_test.go @@ -84,7 +84,7 @@ func TestSimpleImageChangeBuildTrigger(t *testing.T) { func imageChangeBuildConfig() *buildapi.BuildConfig { buildcfg := &buildapi.BuildConfig{ ObjectMeta: kapi.ObjectMeta{ - Name: "testBuildCfg", + Name: "test-build-cfg", }, Parameters: buildapi.BuildParameters{ Source: buildapi.BuildSource{ @@ -101,7 +101,7 @@ func imageChangeBuildConfig() *buildapi.BuildConfig { }, }, Output: buildapi.BuildOutput{ - ImageTag: "tag", + DockerImageReference: "foo:tag", }, }, Triggers: []buildapi.BuildTriggerPolicy{ diff --git a/test/integration/webhookgithub_test.go b/test/integration/webhookgithub_test.go index d0bba3e8c638..8b2204a0f1bf 100644 --- a/test/integration/webhookgithub_test.go +++ b/test/integration/webhookgithub_test.go @@ -50,7 +50,7 @@ func TestWebhookGithubPush(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "namespace/builtimage", + DockerImageReference: "namespace/builtimage", }, }, } @@ -106,7 +106,7 @@ func TestWebhookGithubPing(t *testing.T) { }, }, Output: buildapi.BuildOutput{ - ImageTag: "namespace/builtimage", + DockerImageReference: "namespace/builtimage", }, }, } From 7d705af09388d8a46a9901ebe3ba28ffb992d695 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 20 Jan 2015 00:00:41 -0500 Subject: [PATCH 04/18] When creating build, lookup "to" field if specified Also change builds to go into "Error" status when a problem occurs and record a message. Slightly refactor the build controller code. If the build pod already exists, assume that there is a race in build execution and stay in "pending" state. This means that clients that want to create ImageRepositories and BuildConfigs at the same time that are using the integrated registry only need to specify: POST /imageRepositories { "metadata": { "name": "myimage", }, } POST /buildConfigs { "parameters": { "output": { "to": { "name": "myimage", } } } } and the build will be configured with the right registry name automatically. The integrated registry must be configured properly and OpenShift must be configured to default to it via OPENSHIFT_DEFAULT_REGISTRY. --- pkg/build/api/types.go | 3 + pkg/build/api/v1beta1/types.go | 3 + pkg/build/controller/controller.go | 95 +++++++++++---- pkg/build/controller/controller_test.go | 149 +++++++++++++++++++++--- 4 files changed, 207 insertions(+), 43 deletions(-) diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 0b521a35f452..92d1157e1847 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -16,6 +16,9 @@ type Build struct { // Status is the current status of the build. Status BuildStatus `json:"status,omitempty"` + // A human readable message indicating details about why the build has this status + Message string `json:"message,omitempty"` + // PodName is the name of the pod that is used to execute the build PodName string `json:"podName,omitempty"` diff --git a/pkg/build/api/v1beta1/types.go b/pkg/build/api/v1beta1/types.go index 00bf4847f2c7..d95db87343aa 100644 --- a/pkg/build/api/v1beta1/types.go +++ b/pkg/build/api/v1beta1/types.go @@ -16,6 +16,9 @@ type Build struct { // Status is the current status of the build. Status BuildStatus `json:"status,omitempty"` + // A human readable message indicating details about why the build has this status + Message string `json:"message,omitempty"` + // PodName is the name of the pod that is used to execute the build PodName string `json:"podName,omitempty"` diff --git a/pkg/build/controller/controller.go b/pkg/build/controller/controller.go index f7a15f423afb..e595d953911c 100644 --- a/pkg/build/controller/controller.go +++ b/pkg/build/controller/controller.go @@ -11,6 +11,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" buildapi "github.com/openshift/origin/pkg/build/api" + image "github.com/openshift/origin/pkg/image/api" ) // BuildController watches build resources and manages their state @@ -21,6 +22,8 @@ type BuildController struct { BuildUpdater buildUpdater PodManager podManager BuildStrategy BuildStrategy + + ImageRepositoryClient imageRepositoryClient } // BuildStrategy knows how to create a pod spec for a pod which can execute a build. @@ -37,6 +40,10 @@ type podManager interface { DeletePod(namespace string, pod *kapi.Pod) error } +type imageRepositoryClient interface { + GetImageRepository(namespace, name string) (*image.ImageRepository, error) +} + // Run begins watching and syncing build jobs onto the cluster. func (bc *BuildController) Run() { go util.Forever(func() { bc.HandleBuild(bc.NextBuild()) }, 0) @@ -51,39 +58,77 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) { return } - nextStatus := buildapi.BuildStatusFailed - build.PodName = fmt.Sprintf("build-%s", build.Name) + if err := bc.nextBuildStatus(build); err != nil { + glog.V(4).Infof("Build failed with error %s/%s: %#v", build.Namespace, build.Name, err) + build.Status = buildapi.BuildStatusError + build.Message = err.Error() + } + if _, err := bc.BuildUpdater.UpdateBuild(build.Namespace, build); err != nil { + glog.V(2).Infof("Failed to record changes to build %s/%s: %#v", build.Namespace, build.Name, err) + } +} + +// nextBuildStatus updates build with any appropriate changes, or returns an error if +// the change cannot occur. When returning nil, be sure to set build.Status and optionally +// build.Message. +func (bc *BuildController) nextBuildStatus(build *buildapi.Build) error { // If a cancelling event was triggered for the build, update build status. if build.Cancelled { - glog.V(2).Infof("Cancelling build %s.", build.Name) - nextStatus = buildapi.BuildStatusCancelled - - } else { - - var podSpec *kapi.Pod - var err error - if podSpec, err = bc.BuildStrategy.CreateBuildPod(build); err != nil { - glog.V(2).Infof("Strategy failed to create build pod definition: %v", err) - nextStatus = buildapi.BuildStatusFailed - } else { - - if _, err := bc.PodManager.CreatePod(build.Namespace, podSpec); err != nil { - if !errors.IsAlreadyExists(err) { - glog.V(2).Infof("Failed to create pod for build %s: %#v", build.Name, err) - nextStatus = buildapi.BuildStatusFailed - } - } else { - glog.V(2).Infof("Created build pod: %#v", podSpec) - nextStatus = buildapi.BuildStatusPending + glog.V(4).Infof("Cancelling build %s.", build.Name) + build.Status = buildapi.BuildStatusCancelled + return nil + } + + // lookup the destination from the referenced image repository + spec := build.Parameters.Output.DockerImageReference + if ref := build.Parameters.Output.To; ref != nil { + // TODO: security, ensure that the reference image stream is actually visible + namespace := ref.Namespace + if len(namespace) == 0 { + namespace = build.Namespace + } + + repo, err := bc.ImageRepositoryClient.GetImageRepository(namespace, ref.Name) + if err != nil { + if errors.IsNotFound(err) { + build.Status = buildapi.BuildStatusFailed + build.Message = fmt.Sprintf("The referenced output image repository %s/%s does not exist", namespace, ref.Name) + return nil } + return fmt.Errorf("the referenced output repo %s/%s could not be found by %s/%s: %v", namespace, ref.Name, build.Namespace, build.Name, err) } + spec = repo.Status.DockerImageRepository } - build.Status = nextStatus - if _, err := bc.BuildUpdater.UpdateBuild(build.Namespace, build); err != nil { - glog.V(2).Infof("Failed to update build %s: %#v", build.Name, err) + // set the expected build parameters, which will be saved if no error occurs + build.Status = buildapi.BuildStatusPending + build.PodName = fmt.Sprintf("build-%s", build.Name) + + // override DockerImageReference in the strategy for the copy we send to the server + copy, err := kapi.Scheme.Copy(build) + if err != nil { + return fmt.Errorf("unable to copy build: %v", err) + } + buildCopy := copy.(*buildapi.Build) + buildCopy.Parameters.Output.DockerImageReference = spec + + // invoke the strategy to get a build pod + podSpec, err := bc.BuildStrategy.CreateBuildPod(buildCopy) + if err != nil { + return fmt.Errorf("the strategy failed to create a build pod for %s/%s: %v", build.Namespace, build.Name, err) + } + + if _, err := bc.PodManager.CreatePod(build.Namespace, podSpec); err != nil { + if errors.IsAlreadyExists(err) { + glog.V(4).Infof("Build pod already existed: %#v", podSpec) + return nil + } + return fmt.Errorf("failed to create pod for build %s/%s: s", build.Namespace, build.Name, err) } + + glog.V(4).Infof("Created pod for build: %#v", podSpec) + return nil } func (bc *BuildController) HandlePod(pod *kapi.Pod) { diff --git a/pkg/build/controller/controller_test.go b/pkg/build/controller/controller_test.go index 0faea2f41c3c..99b73cbf3188 100644 --- a/pkg/build/controller/controller_test.go +++ b/pkg/build/controller/controller_test.go @@ -9,6 +9,7 @@ import ( buildapi "github.com/openshift/origin/pkg/build/api" buildtest "github.com/openshift/origin/pkg/build/controller/test" + imageapi "github.com/openshift/origin/pkg/image/api" ) type okBuildUpdater struct{} @@ -23,9 +24,12 @@ func (ebu *errBuildUpdater) UpdateBuild(namespace string, build *buildapi.Build) return &buildapi.Build{}, errors.New("UpdateBuild error!") } -type okStrategy struct{} +type okStrategy struct { + build *buildapi.Build +} func (os *okStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { + os.build = build return &kapi.Pod{}, nil } @@ -65,10 +69,34 @@ func (_ *errExistsPodManager) DeletePod(namespace string, pod *kapi.Pod) error { return kerrors.NewNotFound("kind", "name") } -func mockBuildAndController(status buildapi.BuildStatus) (build *buildapi.Build, controller *BuildController) { +type okImageRepositoryClient struct{} + +func (_ *okImageRepositoryClient) GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error) { + return &imageapi.ImageRepository{ + ObjectMeta: kapi.ObjectMeta{Name: name, Namespace: namespace}, + Status: imageapi.ImageRepositoryStatus{ + DockerImageRepository: "image/repo", + }, + }, nil +} + +type errImageRepositoryClient struct{} + +func (_ *errImageRepositoryClient) GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error) { + return nil, errors.New("GetImageRepository error!") +} + +type errNotFoundImageRepositoryClient struct{} + +func (_ *errNotFoundImageRepositoryClient) GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error) { + return nil, kerrors.NewNotFound("ImageRepository", name) +} + +func mockBuildAndController(status buildapi.BuildStatus, output buildapi.BuildOutput) (build *buildapi.Build, controller *BuildController) { build = &buildapi.Build{ ObjectMeta: kapi.ObjectMeta{ - Name: "dataBuild", + Name: "data-build", + Namespace: "namespace", Labels: map[string]string{ "name": "dataBuild", }, @@ -86,20 +114,19 @@ func mockBuildAndController(status buildapi.BuildStatus) (build *buildapi.Build, ContextDir: "contextimage", }, }, - Output: buildapi.BuildOutput{ - DockerImageReference: "repository/dataBuild", - }, + Output: output, }, Status: status, PodName: "-the-pod-id", } controller = &BuildController{ - BuildStore: buildtest.NewFakeBuildStore(build), - BuildUpdater: &okBuildUpdater{}, - PodManager: &okPodManager{}, - NextBuild: func() *buildapi.Build { return nil }, - NextPod: func() *kapi.Pod { return nil }, - BuildStrategy: &okStrategy{}, + BuildStore: buildtest.NewFakeBuildStore(build), + BuildUpdater: &okBuildUpdater{}, + PodManager: &okPodManager{}, + NextBuild: func() *buildapi.Build { return nil }, + NextPod: func() *kapi.Pod { return nil }, + BuildStrategy: &okStrategy{}, + ImageRepositoryClient: &okImageRepositoryClient{}, } return } @@ -124,60 +151,134 @@ func TestHandleBuild(t *testing.T) { type handleBuildTest struct { inStatus buildapi.BuildStatus outStatus buildapi.BuildStatus + buildOutput buildapi.BuildOutput buildStrategy BuildStrategy buildUpdater buildUpdater + imageClient imageRepositoryClient podManager podManager + outputSpec string } tests := []handleBuildTest{ { // 0 inStatus: buildapi.BuildStatusNew, outStatus: buildapi.BuildStatusPending, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 1 inStatus: buildapi.BuildStatusPending, outStatus: buildapi.BuildStatusPending, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 2 inStatus: buildapi.BuildStatusRunning, outStatus: buildapi.BuildStatusRunning, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 3 inStatus: buildapi.BuildStatusComplete, outStatus: buildapi.BuildStatusComplete, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 4 inStatus: buildapi.BuildStatusFailed, outStatus: buildapi.BuildStatusFailed, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 5 inStatus: buildapi.BuildStatusError, outStatus: buildapi.BuildStatusError, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 6 inStatus: buildapi.BuildStatusNew, - outStatus: buildapi.BuildStatusFailed, + outStatus: buildapi.BuildStatusError, buildStrategy: &errStrategy{}, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 7 inStatus: buildapi.BuildStatusNew, - outStatus: buildapi.BuildStatusFailed, + outStatus: buildapi.BuildStatusError, podManager: &errPodManager{}, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 8 inStatus: buildapi.BuildStatusNew, - outStatus: buildapi.BuildStatusFailed, + outStatus: buildapi.BuildStatusPending, podManager: &errExistsPodManager{}, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, }, { // 9 inStatus: buildapi.BuildStatusNew, outStatus: buildapi.BuildStatusPending, buildUpdater: &errBuildUpdater{}, + buildOutput: buildapi.BuildOutput{ + DockerImageReference: "repository/dataBuild", + }, + }, + { // 10 + inStatus: buildapi.BuildStatusNew, + outStatus: buildapi.BuildStatusPending, + buildOutput: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "foo", + }, + }, + outputSpec: "image/repo", + }, + { // 11 + inStatus: buildapi.BuildStatusNew, + outStatus: buildapi.BuildStatusPending, + buildOutput: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "foo", + Namespace: "bar", + }, + }, + outputSpec: "image/repo", + }, + { // 12 + inStatus: buildapi.BuildStatusNew, + outStatus: buildapi.BuildStatusFailed, + imageClient: &errNotFoundImageRepositoryClient{}, + buildOutput: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "foo", + }, + }, + }, + { // 13 + inStatus: buildapi.BuildStatusNew, + outStatus: buildapi.BuildStatusError, + imageClient: &errImageRepositoryClient{}, + buildOutput: buildapi.BuildOutput{ + To: &kapi.ObjectReference{ + Name: "foo", + }, + }, }, } for i, tc := range tests { - build, ctrl := mockBuildAndController(tc.inStatus) + build, ctrl := mockBuildAndController(tc.inStatus, tc.buildOutput) if tc.buildStrategy != nil { ctrl.BuildStrategy = tc.buildStrategy } @@ -187,12 +288,24 @@ func TestHandleBuild(t *testing.T) { if tc.podManager != nil { ctrl.PodManager = tc.podManager } + if tc.imageClient != nil { + ctrl.ImageRepositoryClient = tc.imageClient + } ctrl.HandleBuild(build) if build.Status != tc.outStatus { t.Errorf("(%d) Expected %s, got %s!", i, tc.outStatus, build.Status) } + if tc.inStatus != buildapi.BuildStatusError && build.Status == buildapi.BuildStatusError && len(build.Message) == 0 { + t.Errorf("(%d) errored build should set message: %#v", i, build) + } + if len(tc.outputSpec) != 0 { + build := ctrl.BuildStrategy.(*okStrategy).build + if build.Parameters.Output.DockerImageReference != tc.outputSpec { + t.Errorf("(%d) expected build sent to strategy to have docker spec %q: %#v", i, tc.outputSpec, build) + } + } } } @@ -253,7 +366,7 @@ func TestHandlePod(t *testing.T) { } for i, tc := range tests { - build, ctrl := mockBuildAndController(tc.inStatus) + build, ctrl := mockBuildAndController(tc.inStatus, buildapi.BuildOutput{}) pod := mockPod(tc.podStatus, tc.exitCode) if tc.matchID { build.PodName = pod.Name @@ -311,7 +424,7 @@ func TestCancelBuild(t *testing.T) { } for i, tc := range tests { - build, ctrl := mockBuildAndController(tc.inStatus) + build, ctrl := mockBuildAndController(tc.inStatus, buildapi.BuildOutput{}) pod := mockPod(tc.podStatus, tc.exitCode) ctrl.CancelBuild(build, pod) From 808bae1a1ddec4564717a4677bf38ccd4ca30737 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 20 Jan 2015 13:41:30 -0500 Subject: [PATCH 05/18] Review comments --- pkg/build/api/types.go | 7 ++++--- pkg/build/api/v1beta1/types.go | 3 ++- pkg/build/api/validation/validation.go | 4 ++-- pkg/build/builder/docker.go | 7 ++++--- pkg/build/builder/sti.go | 7 ++++--- pkg/build/builder/util.go | 6 ------ pkg/build/builder/util_test.go | 2 +- pkg/build/controller/controller.go | 11 ++++++----- pkg/build/controller/controller_test.go | 2 +- pkg/build/controller/strategy/util.go | 6 +++--- 10 files changed, 27 insertions(+), 28 deletions(-) diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 92d1157e1847..5b9424e696c9 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -213,7 +213,7 @@ type BuildOutput struct { // may be empty, in which case the named ImageRepository will be retrieved from the namespace // of the build. Kind must be set to 'ImageRepository' and is the only supported value. If set, // this field takes priority over DockerImageReference. This value will be used to look up - // a Docker image repository to push to. + // a Docker image repository to push to. Failure to find the To will result in a build error. To *kapi.ObjectReference `json:"to,omitempty"` // Tag is the "version name" that will be associated with the output image. This @@ -223,7 +223,7 @@ type BuildOutput struct { Tag string `json:"tag,omitempty"` // DockerImageReference is the full name of an image ([registry/]name[:tag]), and will be the - // value sent to Docker push at the end of a build. + // value sent to Docker push at the end of a build if the To field is not defined. DockerImageReference string `json:"dockerImageReference,omitempty"` } @@ -240,7 +240,8 @@ type BuildConfig struct { // are defined, a new build can only occur as a result of an explicit client build creation. Triggers []BuildTriggerPolicy `json:"triggers,omitempty"` - // Parameters holds all the input necessary to produce a new build. + // Parameters holds all the input necessary to produce a new build. A build config may only + // define either the Output.To or Output.DockerImageReference fields, but not both. Parameters BuildParameters `json:"parameters,omitempty"` } diff --git a/pkg/build/api/v1beta1/types.go b/pkg/build/api/v1beta1/types.go index d95db87343aa..12e5ac2ee899 100644 --- a/pkg/build/api/v1beta1/types.go +++ b/pkg/build/api/v1beta1/types.go @@ -244,7 +244,8 @@ type BuildConfig struct { // are defined, a new build can only occur as a result of an explicit client build creation. Triggers []BuildTriggerPolicy `json:"triggers,omitempty"` - // Parameters holds all the input necessary to produce a new build. + // Parameters holds all the input necessary to produce a new build. A build config may only + // define either the Output.To or Output.DockerImageReference fields, but not both. Parameters BuildParameters `json:"parameters,omitempty"` } diff --git a/pkg/build/api/validation/validation.go b/pkg/build/api/validation/validation.go index 3ec474115d61..4737f342b627 100644 --- a/pkg/build/api/validation/validation.go +++ b/pkg/build/api/validation/validation.go @@ -8,7 +8,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" buildapi "github.com/openshift/origin/pkg/build/api" - image "github.com/openshift/origin/pkg/image/api" + imageapi "github.com/openshift/origin/pkg/image/api" ) // ValidateBuild tests required fields for a Build. @@ -126,7 +126,7 @@ func validateOutput(output *buildapi.BuildOutput) errs.ValidationErrorList { } if len(output.DockerImageReference) != 0 { - if _, _, _, _, err := image.SplitDockerPullSpec(output.DockerImageReference); err != nil { + if _, _, _, _, err := imageapi.SplitDockerPullSpec(output.DockerImageReference); err != nil { allErrs = append(allErrs, errs.NewFieldInvalid("dockerImageReference", output.DockerImageReference, err.Error())) } } diff --git a/pkg/build/builder/docker.go b/pkg/build/builder/docker.go index 675760060ca2..a41d2c3a66b1 100644 --- a/pkg/build/builder/docker.go +++ b/pkg/build/builder/docker.go @@ -64,9 +64,10 @@ func (d *DockerBuilder) Build() error { if err = d.dockerBuild(buildDir); err != nil { return err } - defer removeImage(d.dockerClient, imageTag(d.build)) + tag := d.build.Parameters.Output.DockerImageReference + defer removeImage(d.dockerClient, tag) if len(d.build.Parameters.Output.DockerImageReference) != 0 { - return pushImage(d.dockerClient, imageTag(d.build), d.auth) + return pushImage(d.dockerClient, tag, d.auth) } return nil } @@ -175,5 +176,5 @@ func (d *DockerBuilder) dockerBuild(dir string) error { } noCache = d.build.Parameters.Strategy.DockerStrategy.NoCache } - return buildImage(d.dockerClient, dir, noCache, imageTag(d.build), d.tar) + return buildImage(d.dockerClient, dir, noCache, d.build.Parameters.Output.DockerImageReference, d.tar) } diff --git a/pkg/build/builder/sti.go b/pkg/build/builder/sti.go index 04d4ccbf62ef..95c63d0ef8e8 100644 --- a/pkg/build/builder/sti.go +++ b/pkg/build/builder/sti.go @@ -29,11 +29,12 @@ func NewSTIBuilder(client DockerClient, dockerSocket string, authCfg docker.Auth // Build executes the STI build func (s *STIBuilder) Build() error { + tag := s.build.Parameters.Output.DockerImageReference request := &stiapi.Request{ BaseImage: s.build.Parameters.Strategy.STIStrategy.Image, DockerSocket: s.dockerSocket, Source: s.build.Parameters.Source.Git.URI, - Tag: imageTag(s.build), + Tag: tag, ScriptsURL: s.build.Parameters.Strategy.STIStrategy.Scripts, Environment: getBuildEnvVars(s.build), Clean: s.build.Parameters.Strategy.STIStrategy.Clean, @@ -48,12 +49,12 @@ func (s *STIBuilder) Build() error { if err != nil { return err } - defer removeImage(s.dockerClient, imageTag(s.build)) + defer removeImage(s.dockerClient, tag) if _, err = builder.Build(); err != nil { return err } if len(s.build.Parameters.Output.DockerImageReference) != 0 { - return pushImage(s.dockerClient, imageTag(s.build), s.auth) + return pushImage(s.dockerClient, tag, s.auth) } return nil } diff --git a/pkg/build/builder/util.go b/pkg/build/builder/util.go index 3d821d952351..c3870a77d51f 100644 --- a/pkg/build/builder/util.go +++ b/pkg/build/builder/util.go @@ -8,12 +8,6 @@ type Builder interface { Build() error } -// imageTag returns the tag to be used for the build. If a registry has been -// specified, it will prepend the registry to the name -func imageTag(build *api.Build) string { - return build.Parameters.Output.DockerImageReference -} - // getBuildEnvVars returns a map with the environment variables that should be added // to the built image func getBuildEnvVars(build *api.Build) map[string]string { diff --git a/pkg/build/builder/util_test.go b/pkg/build/builder/util_test.go index eed2f836f9d4..993368354d2a 100644 --- a/pkg/build/builder/util_test.go +++ b/pkg/build/builder/util_test.go @@ -35,7 +35,7 @@ func TestImageTag(t *testing.T) { }, } for _, x := range tests { - result := imageTag(&x.build) + result := x.build.Parameters.Output.DockerImageReference if result != x.expected { t.Errorf("Unexpected imageTag result. Expected: %s, Actual: %s", result, x.expected) diff --git a/pkg/build/controller/controller.go b/pkg/build/controller/controller.go index e595d953911c..4ce6dc2aa787 100644 --- a/pkg/build/controller/controller.go +++ b/pkg/build/controller/controller.go @@ -11,7 +11,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" buildapi "github.com/openshift/origin/pkg/build/api" - image "github.com/openshift/origin/pkg/image/api" + imageapi "github.com/openshift/origin/pkg/image/api" ) // BuildController watches build resources and manages their state @@ -41,7 +41,7 @@ type podManager interface { } type imageRepositoryClient interface { - GetImageRepository(namespace, name string) (*image.ImageRepository, error) + GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error) } // Run begins watching and syncing build jobs onto the cluster. @@ -59,6 +59,9 @@ func (bc *BuildController) HandleBuild(build *buildapi.Build) { } if err := bc.nextBuildStatus(build); err != nil { + // TODO: all build errors should be retried, and build error should not be a permanent status change. + // Instead, we should requeue this build request using the same backoff logic as the scheduler. + // BuildStatusError should be reserved for meaning "permanently errored, no way to try again". glog.V(4).Infof("Build failed with error %s/%s: %#v", build.Namespace, build.Name, err) build.Status = buildapi.BuildStatusError build.Message = err.Error() @@ -92,9 +95,7 @@ func (bc *BuildController) nextBuildStatus(build *buildapi.Build) error { repo, err := bc.ImageRepositoryClient.GetImageRepository(namespace, ref.Name) if err != nil { if errors.IsNotFound(err) { - build.Status = buildapi.BuildStatusFailed - build.Message = fmt.Sprintf("The referenced output image repository %s/%s does not exist", namespace, ref.Name) - return nil + return fmt.Errorf("the referenced output image repository %s/%s does not exist", namespace, ref.Name) } return fmt.Errorf("the referenced output repo %s/%s could not be found by %s/%s: %v", namespace, ref.Name, build.Namespace, build.Name, err) } diff --git a/pkg/build/controller/controller_test.go b/pkg/build/controller/controller_test.go index 99b73cbf3188..0d25408b98d4 100644 --- a/pkg/build/controller/controller_test.go +++ b/pkg/build/controller/controller_test.go @@ -257,7 +257,7 @@ func TestHandleBuild(t *testing.T) { }, { // 12 inStatus: buildapi.BuildStatusNew, - outStatus: buildapi.BuildStatusFailed, + outStatus: buildapi.BuildStatusError, // TODO: this should be a retry imageClient: &errNotFoundImageRepositoryClient{}, buildOutput: buildapi.BuildOutput{ To: &kapi.ObjectReference{ diff --git a/pkg/build/controller/strategy/util.go b/pkg/build/controller/strategy/util.go index ae875550d124..28d15ad91f65 100644 --- a/pkg/build/controller/strategy/util.go +++ b/pkg/build/controller/strategy/util.go @@ -6,7 +6,7 @@ import ( kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" buildapi "github.com/openshift/origin/pkg/build/api" - image "github.com/openshift/origin/pkg/image/api" + imageapi "github.com/openshift/origin/pkg/image/api" ) // dockerSocketPath is the default path for the Docker socket inside the builder @@ -77,8 +77,8 @@ func setupBuildEnv(build *buildapi.Build, pod *kapi.Pod) { // Do nothing for unknown source types } - if registry, namespace, name, tag, err := image.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference); err == nil { - outputImage := image.JoinDockerPullSpec("", namespace, name, tag) + if registry, namespace, name, tag, err := imageapi.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference); err == nil { + outputImage := imageapi.JoinDockerPullSpec("", namespace, name, tag) vars = append(vars, kapi.EnvVar{"OUTPUT_IMAGE", outputImage}) vars = append(vars, kapi.EnvVar{"OUTPUT_REGISTRY", registry}) } From 54d5da572f47aa7a977ee6543454fc9f4bb79c0e Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 22 Jan 2015 15:29:36 -0500 Subject: [PATCH 06/18] Check for an ImageRepositoryMapping with metadata/name before DIR We want to phase out DockerImageRepository over time. --- pkg/image/registry/imagerepositorymapping/rest.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/image/registry/imagerepositorymapping/rest.go b/pkg/image/registry/imagerepositorymapping/rest.go index d8c9873b3cb2..fce2041303ca 100644 --- a/pkg/image/registry/imagerepositorymapping/rest.go +++ b/pkg/image/registry/imagerepositorymapping/rest.go @@ -87,6 +87,9 @@ func (s *REST) Create(ctx kapi.Context, obj runtime.Object) (<-chan apiserver.RE // findRepositoryForMapping retrieves an ImageRepository whose DockerImageRepository matches dockerRepo. func (s *REST) findRepositoryForMapping(ctx kapi.Context, mapping *api.ImageRepositoryMapping) (*api.ImageRepository, error) { + if len(mapping.Name) > 0 { + return s.imageRepositoryRegistry.GetImageRepository(ctx, mapping.Name) + } if len(mapping.DockerImageRepository) != 0 { //TODO make this more efficient list, err := s.imageRepositoryRegistry.ListImageRepositories(ctx, labels.Everything()) @@ -102,7 +105,7 @@ func (s *REST) findRepositoryForMapping(ctx kapi.Context, mapping *api.ImageRepo errors.NewFieldNotFound("dockerImageRepository", mapping.DockerImageRepository), }) } - return s.imageRepositoryRegistry.GetImageRepository(ctx, mapping.Name) + return nil, errors.NewNotFound("ImageRepository", "") } // Update is not supported. From c4fe3618ddf6a27a2cdebde8b8d8afd4ad583446 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 22 Jan 2015 15:30:42 -0500 Subject: [PATCH 07/18] Use Status.DockerImageRepository from CLI --- pkg/cmd/cli/describe/describer.go | 2 +- pkg/cmd/cli/describe/printer.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/cli/describe/describer.go b/pkg/cmd/cli/describe/describer.go index 12d4279e955e..d950c6ef96d1 100644 --- a/pkg/cmd/cli/describe/describer.go +++ b/pkg/cmd/cli/describe/describer.go @@ -242,7 +242,7 @@ func (d *ImageRepositoryDescriber) Describe(namespace, name string) (string, err return tabbedString(func(out *tabwriter.Writer) error { formatMeta(out, imageRepository.ObjectMeta) formatString(out, "Tags", formatLabels(imageRepository.Tags)) - formatString(out, "Registry", imageRepository.DockerImageRepository) + formatString(out, "Registry", imageRepository.Status.DockerImageRepository) return nil }) } diff --git a/pkg/cmd/cli/describe/printer.go b/pkg/cmd/cli/describe/printer.go index 53d43639fedc..a8778405a658 100644 --- a/pkg/cmd/cli/describe/printer.go +++ b/pkg/cmd/cli/describe/printer.go @@ -119,7 +119,7 @@ func printImageRepository(repo *imageapi.ImageRepository, w io.Writer) error { } tags = strings.Join(t, ",") } - _, err := fmt.Fprintf(w, "%s\t%s\t%s\n", repo.Name, repo.DockerImageRepository, tags) + _, err := fmt.Fprintf(w, "%s\t%s\t%s\n", repo.Name, repo.Status.DockerImageRepository, tags) return err } From e5b90d993f51dd91521741d79312a4ac49d9221c Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Tue, 20 Jan 2015 21:26:55 -0500 Subject: [PATCH 08/18] Support service variable substitution in docker registry variable At runtime, dynamically load and cache the value of OPENSHIFT_DOCKER_REGISTRY by allowing environment substitution that matches service behavior. The registry must be in the default namespace and the default value is now: OPENSHIFT_DOCKER_REGISTRY='${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}' If the variables do not resolve (for instance, the service does not exist yet), then the server will act as if there is no default registry and will return empty string in Status.DockerImageRepository for ImageRepositories that don't have the DockerImageRepository field set (meaning they don't "follow" a remote repository). Once the string is loaded it is remembered until restart, so deleting and restarting the default 'docker-registry' service will result in the status field being unavailable. In the future, OpenShift may create an empty docker-registry service by default on startup that can then be matched to real pods. --- hack/test-cmd.sh | 16 +- hack/test-end-to-end.sh | 15 +- pkg/cmd/server/origin/master.go | 14 +- pkg/image/api/helper.go | 5 +- pkg/image/api/v1beta1/types.go | 2 +- pkg/image/registry/etcd/etcd.go | 50 +++++- pkg/image/registry/etcd/etcd_test.go | 16 +- pkg/image/registry/imagerepository/rest.go | 39 +---- .../registry/imagerepository/rest_test.go | 42 ++++-- pkg/service/environmentresolvercache.go | 142 ++++++++++++++++++ pkg/service/environmentresolvercache_test.go | 83 ++++++++++ test/integration/deploy_trigger_test.go | 31 ++-- .../fixtures/test-image-repository.json | 1 - test/integration/fixtures/test-mapping.json | 6 +- test/integration/imageclient_test.go | 4 +- 15 files changed, 376 insertions(+), 90 deletions(-) create mode 100644 pkg/service/environmentresolvercache.go create mode 100644 pkg/service/environmentresolvercache_test.go diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 9bd506ab572b..114950923395 100755 --- a/hack/test-cmd.sh +++ b/hack/test-cmd.sh @@ -48,13 +48,17 @@ out=$(openshift version) echo openshift: $out # Start openshift -openshift start --master="${API_SCHEME}://${API_HOST}:${API_PORT}" --listen="${API_SCHEME}://0.0.0.0:${API_PORT}" --volume-dir="${VOLUME_DIR}" --etcd-dir="${ETCD_DATA_DIR}" --cert-dir="${CERT_DIR}" 1>&2 & -OS_PID=$! - if [[ "$API_SCHEME" == "https" ]]; then - export CURL_CA_BUNDLE="$CERT_DIR/admin/root.crt" + export CURL_CA_BUNDLE="$CERT_DIR/admin/root.crt" fi +openshift start --master="${API_SCHEME}://${API_HOST}:${API_PORT}" --listen="${API_SCHEME}://0.0.0.0:${API_PORT}" --hostname=${API_HOST} --volume-dir="${VOLUME_DIR}" --cert-dir="${CERT_DIR}" --etcd-dir="${ETCD_DATA_DIR}" 1>&2 & +OS_PID=$! + +wait_for_url "http://localhost:${KUBELET_PORT}/healthz" "kubelet: " 0.2 60 +wait_for_url "http://${API_HOST}:${API_PORT}/healthz" "apiserver: " 0.2 60 +wait_for_url "http://${API_HOST}:${API_PORT}/api/v1beta1/minions/127.0.0.1" "apiserver(minions): " 0.2 60 + wait_for_url "${KUBELET_SCHEME}://${API_HOST}:${KUBELET_PORT}/healthz" "kubelet: " 1 30 wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz" "apiserver: " @@ -90,6 +94,10 @@ echo "images: ok" osc get imageRepositories osc create -f test/integration/fixtures/test-image-repository.json +[ -z "$(osc get imageRepositories test -t "{{.status.dockerImageRepository}}")" ] +osc apply -f examples/sample-app/docker-registry-config.json +[ -n "$(osc get imageRepositories test -t "{{.status.dockerImageRepository}}")" ] +osc delete -f examples/sample-app/docker-registry-config.json osc delete imageRepositories test echo "imageRepositories: ok" diff --git a/hack/test-end-to-end.sh b/hack/test-end-to-end.sh index b2d62575a631..cca78423f34a 100755 --- a/hack/test-end-to-end.sh +++ b/hack/test-end-to-end.sh @@ -175,19 +175,19 @@ fi # Deploy private docker registry echo "[INFO] Submitting docker-registry template file for processing" -osc process -n test -f examples/sample-app/docker-registry-template.json -v "OPENSHIFT_MASTER=$API_SCHEME://${CONTAINER_ACCESSIBLE_API_HOST}:$API_PORT,OPENSHIFT_CA_DATA=${OPENSHIFT_CA_DATA},OPENSHIFT_CERT_DATA=${OPENSHIFT_CERT_DATA},OPENSHIFT_KEY_DATA=${OPENSHIFT_KEY_DATA}" > "$ARTIFACT_DIR/docker-registry-config.json" +osc process -f examples/sample-app/docker-registry-template.json -v "OPENSHIFT_MASTER=$API_SCHEME://${CONTAINER_ACCESSIBLE_API_HOST}:$API_PORT,OPENSHIFT_CA_DATA=${OPENSHIFT_CA_DATA},OPENSHIFT_CERT_DATA=${OPENSHIFT_CERT_DATA},OPENSHIFT_KEY_DATA=${OPENSHIFT_KEY_DATA}" > "$ARTIFACT_DIR/docker-registry-config.json" echo "[INFO] Deploying private Docker registry from $ARTIFACT_DIR/docker-registry-config.json" -osc apply -n test -f ${ARTIFACT_DIR}/docker-registry-config.json +osc apply -f ${ARTIFACT_DIR}/docker-registry-config.json echo "[INFO] Waiting for Docker registry pod to start" -wait_for_command "osc get -n test pods | grep registrypod | grep -i Running" $((5*TIME_MIN)) +wait_for_command "osc get pods | grep registrypod | grep -i Running" $((5*TIME_MIN)) echo "[INFO] Waiting for Docker registry service to start" -wait_for_command "osc get -n test services | grep registrypod" +wait_for_command "osc get services | grep registrypod" # services can end up on any IP. Make sure we get the IP we need for the docker registry -DOCKER_REGISTRY_IP=$(osc get -n test -o template --output-version=v1beta1 --template="{{ .portalIP }}" service docker-registry) +DOCKER_REGISTRY_IP=$(osc get -o template --output-version=v1beta1 --template="{{ .portalIP }}" service docker-registry) echo "[INFO] Probing the docker-registry" wait_for_url_timed "http://${DOCKER_REGISTRY_IP}:5001" "[INFO] Docker registry says: " $((2*TIME_MIN)) @@ -206,11 +206,6 @@ osc process -n test -f examples/sample-app/application-template-stibuild.json > osc process -n docker -f examples/sample-app/application-template-dockerbuild.json > "${DOCKER_CONFIG_FILE}" osc process -n custom -f examples/sample-app/application-template-custombuild.json > "${CUSTOM_CONFIG_FILE}" -# substitute the default IP address with the address where we actually ended up -# TODO: make this be unnecessary by fixing images -# This is no longer needed because the docker registry explicitly requests the 172.30.17.3 ip address. -#sed -i "s,172.30.17.3,${DOCKER_REGISTRY_IP},g" "${CONFIG_FILE}" - echo "[INFO] Applying STI application config" osc apply -n test -f "${STI_CONFIG_FILE}" diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index 21f20855225d..632185fa8c41 100644 --- a/pkg/cmd/server/origin/master.go +++ b/pkg/cmd/server/origin/master.go @@ -65,6 +65,7 @@ import ( projectregistry "github.com/openshift/origin/pkg/project/registry/project" routeetcd "github.com/openshift/origin/pkg/route/registry/etcd" routeregistry "github.com/openshift/origin/pkg/route/registry/route" + "github.com/openshift/origin/pkg/service" templateregistry "github.com/openshift/origin/pkg/template/registry" "github.com/openshift/origin/pkg/user" useretcd "github.com/openshift/origin/pkg/user/registry/etcd" @@ -191,8 +192,15 @@ func (c *MasterConfig) EnsureCORSAllowedOrigins(origins []string) { } func (c *MasterConfig) InstallAPI(container *restful.Container) []string { + defaultRegistry := env("OPENSHIFT_DEFAULT_REGISTRY", "${DOCKER_REGISTRY_SERVICE_HOST}:${DOCKER_REGISTRY_SERVICE_PORT}") + svcCache := service.NewServiceResolverCache(c.KubeClient().Services(api.NamespaceDefault).Get) + defaultRegistryFunc, err := svcCache.Defer(defaultRegistry) + if err != nil { + glog.Fatalf("OPENSHIFT_DEFAULT_REGISTRY variable is invalid %q: %v", defaultRegistry, err) + } + buildEtcd := buildetcd.New(c.EtcdHelper) - imageEtcd := imageetcd.New(c.EtcdHelper) + imageEtcd := imageetcd.New(c.EtcdHelper, imageetcd.DefaultRegistryFunc(defaultRegistryFunc)) deployEtcd := deployetcd.New(c.EtcdHelper) routeEtcd := routeetcd.New(c.EtcdHelper) projectEtcd := projectetcd.New(c.EtcdHelper) @@ -211,8 +219,6 @@ func (c *MasterConfig) InstallAPI(container *restful.Container) []string { rollbackDeploymentGetter := &clientDeploymentInterface{kclient} rollbackDeploymentConfigGetter := &clientDeploymentConfigInterface{osclient} - defaultRegistry := env("OPENSHIFT_DEFAULT_REGISTRY", "") - // initialize OpenShift API storage := map[string]apiserver.RESTStorage{ "builds": buildregistry.NewREST(buildEtcd), @@ -220,7 +226,7 @@ func (c *MasterConfig) InstallAPI(container *restful.Container) []string { "buildLogs": buildlogregistry.NewREST(buildEtcd, c.BuildLogClient()), "images": image.NewREST(imageEtcd), - "imageRepositories": imagerepository.NewREST(imageEtcd, defaultRegistry), + "imageRepositories": imagerepository.NewREST(imageEtcd), "imageRepositoryMappings": imagerepositorymapping.NewREST(imageEtcd, imageEtcd), "imageRepositoryTags": imagerepositorytag.NewREST(imageEtcd, imageEtcd), diff --git a/pkg/image/api/helper.go b/pkg/image/api/helper.go index 9f78f789cf06..1885607caefd 100644 --- a/pkg/image/api/helper.go +++ b/pkg/image/api/helper.go @@ -52,7 +52,10 @@ func JoinDockerPullSpec(registry, namespace, name, tag string) string { tag = ":" + tag } if len(namespace) == 0 { - return fmt.Sprintf("%s%s", name, tag) + if len(registry) == 0 { + return fmt.Sprintf("%s%s", name, tag) + } + namespace = DockerDefaultNamespace } if len(registry) == 0 { return fmt.Sprintf("%s/%s%s", namespace, name, tag) diff --git a/pkg/image/api/v1beta1/types.go b/pkg/image/api/v1beta1/types.go index a2e6d65c9fb0..d038e255a685 100644 --- a/pkg/image/api/v1beta1/types.go +++ b/pkg/image/api/v1beta1/types.go @@ -54,7 +54,7 @@ type ImageRepository struct { type ImageRepositoryStatus struct { // Represents the effective location this repository may be accessed at. May be empty until the server // determines where the repository is located - DockerImageRepository string `json:"dockerImageRepository,omitempty"` + DockerImageRepository string `json:"dockerImageRepository"` } // TODO add metadata overrides diff --git a/pkg/image/registry/etcd/etcd.go b/pkg/image/registry/etcd/etcd.go index 8c0b1bc2faf4..1b2291afc76b 100644 --- a/pkg/image/registry/etcd/etcd.go +++ b/pkg/image/registry/etcd/etcd.go @@ -26,15 +26,32 @@ const ( ImageRepositoriesPath string = "/imageRepositories" ) +// DefaultRegistry returns the default Docker registry (host or host:port), or false if it is not available. +type DefaultRegistry interface { + DefaultRegistry() (string, bool) +} + +// DefaultRegistryFunc implements DefaultRegistry for a simple function. +type DefaultRegistryFunc func() (string, bool) + +// DefaultRegistry implements the DefaultRegistry interface for a function. +func (fn DefaultRegistryFunc) DefaultRegistry() (string, bool) { + return fn() +} + // Etcd implements ImageRegistry and ImageRepositoryRegistry backed by etcd. type Etcd struct { tools.EtcdHelper + defaultRegistry DefaultRegistry } -// New returns a new etcd registry. -func New(helper tools.EtcdHelper) *Etcd { +// New returns a new etcd registry. Default registry is the value that will be +// applied to the Status.DockerImageRepository field if the repository does not +// have a specified DockerImageRepository. +func New(helper tools.EtcdHelper, defaultRegistry DefaultRegistry) *Etcd { return &Etcd{ - EtcdHelper: helper, + EtcdHelper: helper, + defaultRegistry: defaultRegistry, } } @@ -160,6 +177,7 @@ func (r *Etcd) ListImageRepositories(ctx kapi.Context, selector labels.Selector) filtered := []api.ImageRepository{} for _, item := range list.Items { if selector.Matches(labels.Set(item.Labels)) { + r.fillRepository(&item) filtered = append(filtered, item) } } @@ -185,7 +203,7 @@ func (r *Etcd) GetImageRepository(ctx kapi.Context, id string) (*api.ImageReposi if err = r.ExtractObj(key, &repo, false); err != nil { return nil, etcderr.InterpretGetError(err, "imageRepository", id) } - return &repo, nil + return r.fillRepository(&repo), nil } // WatchImageRepositories begins watching for new, changed, or deleted ImageRepositories. @@ -205,7 +223,11 @@ func (r *Etcd) WatchImageRepositories(ctx kapi.Context, label, field labels.Sele "name": repo.Name, "dockerImageRepository": repo.DockerImageRepository, } - return label.Matches(labels.Set(repo.Labels)) && field.Matches(fields) + if !label.Matches(labels.Set(repo.Labels)) || !field.Matches(fields) { + return false + } + r.fillRepository(repo) + return true }) } @@ -238,3 +260,21 @@ func (r *Etcd) DeleteImageRepository(ctx kapi.Context, id string) error { err = r.Delete(key, false) return etcderr.InterpretDeleteError(err, "imageRepository", id) } + +// fillRepository sets the status information of a repository +func (r *Etcd) fillRepository(repo *api.ImageRepository) *api.ImageRepository { + var value string + if len(repo.DockerImageRepository) != 0 { + value = repo.DockerImageRepository + } else { + registry, ok := r.defaultRegistry.DefaultRegistry() + if ok { + if len(repo.Namespace) == 0 { + repo.Namespace = kapi.NamespaceDefault + } + value = api.JoinDockerPullSpec(registry, repo.Namespace, repo.Name, "") + } + } + repo.Status.DockerImageRepository = value + return repo +} diff --git a/pkg/image/registry/etcd/etcd_test.go b/pkg/image/registry/etcd/etcd_test.go index c8608ab8ed6f..9fc6223d6157 100644 --- a/pkg/image/registry/etcd/etcd_test.go +++ b/pkg/image/registry/etcd/etcd_test.go @@ -55,8 +55,15 @@ func makeTestDefaultImageRepositoriesListKey() string { return makeTestImageRepositoriesListKey(kapi.NamespaceDefault) } +var ( + testDefaultRegistry = DefaultRegistryFunc(func() (string, bool) { return "test", true }) + noDefaultRegistry = DefaultRegistryFunc(func() (string, bool) { + return "", false + }) +) + func NewTestEtcd(client tools.EtcdClient) *Etcd { - return New(tools.EtcdHelper{client, latest.Codec, tools.RuntimeVersionAdapter{latest.ResourceVersioner}}) + return New(tools.EtcdHelper{client, latest.Codec, tools.RuntimeVersionAdapter{latest.ResourceVersioner}}, noDefaultRegistry) } func TestEtcdListImagesEmpty(t *testing.T) { @@ -510,12 +517,13 @@ func TestEtcdListImageRepositoriesEverything(t *testing.T) { E: nil, } registry := NewTestEtcd(fakeClient) + registry.defaultRegistry = testDefaultRegistry repos, err := registry.ListImageRepositories(kapi.NewDefaultContext(), labels.Everything()) if err != nil { t.Errorf("unexpected error: %v", err) } - if len(repos.Items) != 2 || repos.Items[0].Name != "foo" || repos.Items[1].Name != "bar" { + if len(repos.Items) != 2 || repos.Items[0].Name != "foo" || repos.Items[1].Name != "bar" || repos.Items[1].Status.DockerImageRepository != "test/default/bar" { t.Errorf("Unexpected images list: %#v", repos) } } @@ -802,6 +810,8 @@ func TestEtcdWatchImageRepositories(t *testing.T) { fakeClient.WaitForWatchCompletion() for testIndex, repo := range tt.repos { + // Set this value to avoid duplication in tests + repo.Status.DockerImageRepository = repo.DockerImageRepository repoBytes, _ := latest.Codec.Encode(repo) fakeClient.WatchResponse <- &etcd.Response{ Action: "set", @@ -822,7 +832,7 @@ func TestEtcdWatchImageRepositories(t *testing.T) { t.Errorf("Expected %v, got %v", e, a) } if e, a := repo, event.Object; !reflect.DeepEqual(e, a) { - t.Errorf("Expected %v, got %v", e, a) + t.Errorf("Expected %#v, got %#v", e, a) } case <-time.After(50 * time.Millisecond): if tt.expected[testIndex] { diff --git a/pkg/image/registry/imagerepository/rest.go b/pkg/image/registry/imagerepository/rest.go index b3dee36c591a..9f07178f4ca4 100644 --- a/pkg/image/registry/imagerepository/rest.go +++ b/pkg/image/registry/imagerepository/rest.go @@ -16,17 +16,13 @@ import ( // REST implements the RESTStorage interface in terms of an Registry. type REST struct { - registry Registry - defaultRegistry string + registry Registry } -// NewREST returns a new REST. Default registry is the prefix that will be -// applied to the Status.DockerImageRepository field if the repository does not -// have a real DockerImageRepository. -func NewREST(registry Registry, defaultRegistry string) apiserver.RESTStorage { +// NewREST returns a new REST. +func NewREST(registry Registry) apiserver.RESTStorage { return &REST{ - registry: registry, - defaultRegistry: defaultRegistry, + registry: registry, } } @@ -41,24 +37,12 @@ func (*REST) NewList() runtime.Object { // List retrieves a list of ImageRepositories that match selector. func (s *REST) List(ctx kapi.Context, selector, fields labels.Selector) (runtime.Object, error) { - imageRepositories, err := s.registry.ListImageRepositories(ctx, selector) - if err != nil { - return nil, err - } - for i := range imageRepositories.Items { - s.fillRepository(&imageRepositories.Items[i]) - } - return imageRepositories, err + return s.registry.ListImageRepositories(ctx, selector) } // Get retrieves an ImageRepository by id. func (s *REST) Get(ctx kapi.Context, id string) (runtime.Object, error) { - repo, err := s.registry.GetImageRepository(ctx, id) - if err != nil { - return nil, err - } - s.fillRepository(repo) - return repo, nil + return s.registry.GetImageRepository(ctx, id) } // Watch begins watching for new, changed, or deleted ImageRepositories. @@ -114,14 +98,3 @@ func (s *REST) Delete(ctx kapi.Context, id string) (<-chan apiserver.RESTResult, return &kapi.Status{Status: kapi.StatusSuccess}, s.registry.DeleteImageRepository(ctx, id) }), nil } - -// fillRepository sets the status information of a repository -func (s *REST) fillRepository(repo *api.ImageRepository) { - var value string - if len(repo.DockerImageRepository) != 0 { - value = repo.DockerImageRepository - } else { - value = api.JoinDockerPullSpec(s.defaultRegistry, repo.Namespace, repo.Name, "") - } - repo.Status.DockerImageRepository = value -} diff --git a/pkg/image/registry/imagerepository/rest_test.go b/pkg/image/registry/imagerepository/rest_test.go index 500f1ef74955..5540525c6215 100644 --- a/pkg/image/registry/imagerepository/rest_test.go +++ b/pkg/image/registry/imagerepository/rest_test.go @@ -8,7 +8,6 @@ import ( "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" kclient "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/openshift/origin/pkg/image/api" @@ -18,10 +17,12 @@ import ( func TestGetImageRepositoryError(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() mockRepositoryRegistry.Err = fmt.Errorf("test error") - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } image, err := storage.Get(kapi.NewDefaultContext(), "image1") - if image != nil { + if image != (*api.ImageRepository)(nil) { t.Errorf("Unexpected non-nil image: %#v", image) } if err != mockRepositoryRegistry.Err { @@ -35,7 +36,9 @@ func TestGetImageRepositoryOK(t *testing.T) { ObjectMeta: kapi.ObjectMeta{Name: "foo"}, DockerImageRepository: "openshift/ruby-19-centos", } - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } repo, err := storage.Get(kapi.NewDefaultContext(), "foo") if repo == nil { @@ -62,7 +65,7 @@ func TestListImageRepositoriesError(t *testing.T) { t.Errorf("Expected %#v, Got %#v", mockRepositoryRegistry.Err, err) } - if imageRepositories != nil { + if imageRepositories != (*api.ImageRepositoryList)(nil) { t.Errorf("Unexpected non-nil imageRepositories list: %#v", imageRepositories) } } @@ -122,9 +125,11 @@ func TestListImageRepositoriesPopulatedList(t *testing.T) { func TestCreateImageRepositoryOK(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() - storage := NewREST(mockRepositoryRegistry, "test") + storage := REST{ + registry: mockRepositoryRegistry, + } - channel, err := storage.(apiserver.RESTCreater).Create(kapi.NewDefaultContext(), &api.ImageRepository{ObjectMeta: kapi.ObjectMeta{Name: "foo"}}) + channel, err := storage.Create(kapi.NewDefaultContext(), &api.ImageRepository{ObjectMeta: kapi.ObjectMeta{Name: "foo"}}) if err != nil { t.Fatalf("Unexpected non-nil error: %#v", err) } @@ -143,15 +148,14 @@ func TestCreateImageRepositoryOK(t *testing.T) { if repo.DockerImageRepository != "" { t.Errorf("unexpected repository: %#v", repo) } - if repo.Status.DockerImageRepository != "test/default/foo" { - t.Errorf("unexpected Status values: %#v", repo) - } } func TestCreateRegistryErrorSaving(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() mockRepositoryRegistry.Err = fmt.Errorf("foo") - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } channel, err := storage.Create(kapi.NewDefaultContext(), &api.ImageRepository{ObjectMeta: kapi.ObjectMeta{Name: "foo"}}) if err != nil { @@ -182,7 +186,9 @@ func TestUpdateImageRepositoryMissingID(t *testing.T) { func TestUpdateRegistryErrorSaving(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() mockRepositoryRegistry.Err = fmt.Errorf("foo") - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } channel, err := storage.Update(kapi.NewDefaultContext(), &api.ImageRepository{ ObjectMeta: kapi.ObjectMeta{Name: "bar"}, @@ -202,7 +208,9 @@ func TestUpdateRegistryErrorSaving(t *testing.T) { func TestUpdateImageRepositoryOK(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } channel, err := storage.Update(kapi.NewDefaultContext(), &api.ImageRepository{ ObjectMeta: kapi.ObjectMeta{Name: "bar"}, @@ -222,7 +230,9 @@ func TestUpdateImageRepositoryOK(t *testing.T) { func TestDeleteImageRepository(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } channel, err := storage.Delete(kapi.NewDefaultContext(), "foo") if err != nil { @@ -254,7 +264,9 @@ func TestCreateImageRepositoryConflictingNamespace(t *testing.T) { func TestUpdateImageRepositoryConflictingNamespace(t *testing.T) { mockRepositoryRegistry := test.NewImageRepositoryRegistry() - storage := REST{registry: mockRepositoryRegistry} + storage := REST{ + registry: mockRepositoryRegistry, + } channel, err := storage.Update(kapi.WithNamespace(kapi.NewContext(), "legal-name"), &api.ImageRepository{ ObjectMeta: kapi.ObjectMeta{Name: "bar", Namespace: "some-value"}, diff --git a/pkg/service/environmentresolvercache.go b/pkg/service/environmentresolvercache.go new file mode 100644 index 000000000000..80c952663f0f --- /dev/null +++ b/pkg/service/environmentresolvercache.go @@ -0,0 +1,142 @@ +package service + +import ( + "fmt" + "os" + "strconv" + "strings" + "sync" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" +) + +type ServiceRetriever interface { + Get(name string) (*api.Service, error) +} + +type serviceEntry struct { + host string + port string +} + +type ResolverCacheFunc func(name string) (*api.Service, error) + +type ServiceResolverCache struct { + fill ResolverCacheFunc + cache map[string]serviceEntry + lock sync.RWMutex +} + +func NewServiceResolverCache(fill ResolverCacheFunc) *ServiceResolverCache { + return &ServiceResolverCache{ + cache: make(map[string]serviceEntry), + fill: fill, + } +} + +func (c *ServiceResolverCache) get(name string) (host, port string, ok bool) { + // check + c.lock.RLock() + entry, found := c.cache[name] + c.lock.RUnlock() + if found { + return entry.host, entry.port, true + } + + // fill the cache + c.lock.Lock() + defer c.lock.Unlock() + if entry, found := c.cache[name]; found { + return entry.host, entry.port, true + } + service, err := c.fill(name) + if err != nil { + return + } + host, port, ok = service.Spec.PortalIP, strconv.Itoa(service.Spec.Port), true + c.cache[name] = serviceEntry{ + host: host, + port: port, + } + return +} + +func toServiceName(envName string) string { + return strings.TrimSpace(strings.ToLower(strings.Replace(envName, "_", "-", -1))) +} + +func recognizeVariable(name string) (service string, host bool, ok bool) { + switch { + case strings.HasSuffix(name, "_SERVICE_HOST"): + service = toServiceName(strings.TrimSuffix(name, "_SERVICE_HOST")) + host = true + case strings.HasSuffix(name, "_SERVICE_PORT"): + service = toServiceName(strings.TrimSuffix(name, "_SERVICE_PORT")) + default: + return "", false, false + } + if len(service) == 0 { + return "", false, false + } + ok = true + return +} + +func (c *ServiceResolverCache) resolve(name string) (string, bool) { + service, isHost, ok := recognizeVariable(name) + if !ok { + return "", false + } + host, port, ok := c.get(service) + if !ok { + return "", false + } + if isHost { + return host, true + } + return port, true +} + +// Defer takes a string (with optional variables) and an expansion function and returns +// a function that can be called to get the value. This method will optimize the +// expansion away in the event that no expansion is necessary. +func (c *ServiceResolverCache) Defer(env string) (func() (string, bool), error) { + hasExpansion := false + invalid := []string{} + os.Expand(env, func(name string) string { + hasExpansion = true + if _, _, ok := recognizeVariable(name); !ok { + invalid = append(invalid, name) + } + return "" + }) + if len(invalid) != 0 { + return nil, fmt.Errorf("invalid variable name(s): %s", strings.Join(invalid, ", ")) + } + if !hasExpansion { + return func() (string, bool) { return env, true }, nil + } + + // only load the value once + lock := sync.Mutex{} + loaded := false + return func() (string, bool) { + lock.Lock() + defer lock.Unlock() + if loaded { + return env, true + } + resolved := true + expand := os.Expand(env, func(s string) string { + s, ok := c.resolve(s) + resolved = resolved && ok + return s + }) + if !resolved { + return "", false + } + loaded = true + env = expand + return env, true + }, nil +} diff --git a/pkg/service/environmentresolvercache_test.go b/pkg/service/environmentresolvercache_test.go new file mode 100644 index 000000000000..550976259ccf --- /dev/null +++ b/pkg/service/environmentresolvercache_test.go @@ -0,0 +1,83 @@ +package service + +import ( + "testing" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" + "github.com/GoogleCloudPlatform/kubernetes/pkg/client" +) + +func TestServiceResolverCacheEmpty(t *testing.T) { + fakeClient := &client.Fake{} + cache := NewServiceResolverCache(fakeClient.Services("default").Get) + if v, ok := cache.resolve("FOO_SERVICE_HOST"); v != "" || !ok { + t.Errorf("unexpected cache item") + } + if len(fakeClient.Actions) != 1 { + t.Errorf("unexpected client actions: %#v", fakeClient.Actions) + } + cache.resolve("FOO_SERVICE_HOST") + if len(fakeClient.Actions) != 1 { + t.Errorf("unexpected cache miss: %#v", fakeClient.Actions) + } + cache.resolve("FOO_SERVICE_PORT") + if len(fakeClient.Actions) != 1 { + t.Errorf("unexpected cache miss: %#v", fakeClient.Actions) + } +} + +type fakeRetriever struct { + service *api.Service + err error +} + +func (r fakeRetriever) Get(name string) (*api.Service, error) { + return r.service, r.err +} + +func TestServiceResolverCache(t *testing.T) { + c := fakeRetriever{ + err: errors.NewNotFound("Service", "bar"), + } + cache := NewServiceResolverCache(c.Get) + if v, ok := cache.resolve("FOO_SERVICE_HOST"); v != "" || ok { + t.Errorf("unexpected cache item") + } + + c = fakeRetriever{ + service: &api.Service{ + Spec: api.ServiceSpec{ + PortalIP: "127.0.0.1", + Port: 80, + }, + }, + } + cache = NewServiceResolverCache(c.Get) + if v, ok := cache.resolve("FOO_SERVICE_HOST"); v != "127.0.0.1" || !ok { + t.Errorf("unexpected cache item") + } + if v, ok := cache.resolve("FOO_SERVICE_PORT"); v != "80" || !ok { + t.Errorf("unexpected cache item") + } + if _, err := cache.Defer("${UNKNOWN}"); err == nil { + t.Errorf("unexpected non-error") + } + fn, err := cache.Defer("test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v, ok := fn(); v != "test" || !ok { + t.Errorf("unexpected cache item") + } + fn, err = cache.Defer("${FOO_SERVICE_HOST}") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v, ok := fn(); v != "127.0.0.1" || !ok { + t.Errorf("unexpected cache item") + } + if v, ok := fn(); v != "127.0.0.1" || !ok { + t.Errorf("unexpected cache item") + } +} diff --git a/test/integration/deploy_trigger_test.go b/test/integration/deploy_trigger_test.go index d656f69f20cb..bc7086d5b512 100644 --- a/test/integration/deploy_trigger_test.go +++ b/test/integration/deploy_trigger_test.go @@ -51,22 +51,31 @@ func TestSuccessfulManualDeployment(t *testing.T) { config := manualDeploymentConfig() var err error - watch, err := openshift.KubeClient.ReplicationControllers(testNamespace).Watch(labels.Everything(), labels.Everything(), "0") + dc, err := openshift.Client.DeploymentConfigs(testNamespace).Create(config) if err != nil { - t.Fatalf("Couldn't subscribe to Deployments: %v", err) + t.Fatalf("Couldn't create DeploymentConfig: %v %#v", err, config) } - if _, err := openshift.Client.DeploymentConfigs(testNamespace).Create(config); err != nil { - t.Fatalf("Couldn't create DeploymentConfig: %v %#v", err, config) + watch, err := openshift.KubeClient.ReplicationControllers(testNamespace).Watch(labels.Everything(), labels.Everything(), dc.ResourceVersion) + if err != nil { + t.Fatalf("Couldn't subscribe to Deployments: %v", err) } + defer watch.Stop() - if config, err = openshift.Client.DeploymentConfigs(testNamespace).Generate(config.Name); err != nil { + config, err = openshift.Client.DeploymentConfigs(testNamespace).Generate(config.Name) + if err != nil { t.Fatalf("Error generating config: %v", err) } + if config.LatestVersion != 1 { + t.Fatalf("Generated deployment should have version 1: %#v", config) + } + t.Logf("config(1): %#v", config) - if _, err := openshift.Client.DeploymentConfigs(testNamespace).Update(config); err != nil { + new, err := openshift.Client.DeploymentConfigs(testNamespace).Update(config) + if err != nil { t.Fatalf("Couldn't create updated DeploymentConfig: %v %#v", err, config) } + t.Logf("config(2): %#v", new) event := <-watch.ResultChan() if e, a := watchapi.Added, event.Type; e != a { @@ -77,6 +86,9 @@ func TestSuccessfulManualDeployment(t *testing.T) { if e, a := config.Name, deployment.Annotations[deployapi.DeploymentConfigAnnotation]; e != a { t.Fatalf("Expected deployment annotated with deploymentConfig '%s', got '%s'", e, a) } + if e, a := "1", deployment.Annotations[deployapi.DeploymentVersionAnnotation]; e != a { + t.Fatalf("Deployment annotation version does not match: %#v", deployment) + } } func TestSimpleImageChangeTrigger(t *testing.T) { @@ -100,7 +112,8 @@ func TestSimpleImageChangeTrigger(t *testing.T) { t.Fatalf("Couldn't subscribe to Deployments %v", err) } - if imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo); err != nil { + imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo) + if err != nil { t.Fatalf("Couldn't create ImageRepository: %v", err) } @@ -263,7 +276,7 @@ func NewTestOpenshift(t *testing.T) *testOpenshift { interfaces, _ := latest.InterfacesFor(latest.Version) - imageEtcd := imageetcd.New(etcdHelper) + imageEtcd := imageetcd.New(etcdHelper, imageetcd.DefaultRegistryFunc(func() (string, bool) { return "registry:3000", true })) deployEtcd := deployetcd.New(etcdHelper) deployConfigGenerator := &deployconfiggenerator.DeploymentConfigGenerator{ Codec: latest.Codec, @@ -276,7 +289,7 @@ func NewTestOpenshift(t *testing.T) *testOpenshift { storage := map[string]apiserver.RESTStorage{ "images": image.NewREST(imageEtcd), - "imageRepositories": imagerepository.NewREST(imageEtcd, ""), + "imageRepositories": imagerepository.NewREST(imageEtcd), "imageRepositoryMappings": imagerepositorymapping.NewREST(imageEtcd, imageEtcd), "deployments": deployregistry.NewREST(deployEtcd), "deploymentConfigs": deployconfigregistry.NewREST(deployEtcd), diff --git a/test/integration/fixtures/test-image-repository.json b/test/integration/fixtures/test-image-repository.json index 62c257129f88..0755d177cc3f 100644 --- a/test/integration/fixtures/test-image-repository.json +++ b/test/integration/fixtures/test-image-repository.json @@ -7,7 +7,6 @@ "color": "blue" } }, - "dockerImageRepository": "openshift/ruby-19-centos", "tags": { "latest": "foo", "another": "bar", diff --git a/test/integration/fixtures/test-mapping.json b/test/integration/fixtures/test-mapping.json index d13158682abb..ae0327709fcf 100644 --- a/test/integration/fixtures/test-mapping.json +++ b/test/integration/fixtures/test-mapping.json @@ -1,7 +1,9 @@ { + "metadata": { + "name": "test" + }, "kind": "ImageRepositoryMapping", "apiVersion": "v1beta1", - "dockerImageRepository": "openshift/ruby-19-centos", "image": { "kind": "Image", "version": "v1beta1", @@ -9,7 +11,7 @@ "name": "abcd1234" }, "dockerImageReference": "openshift/ruby-19-centos:latest", - "meta": { + "dockerImageMetadata": { "Architecture": "amd64", "Author": "Michal Fojtik \u003cmfojtik@redhat.com\u003e", "Comment": "", diff --git a/test/integration/imageclient_test.go b/test/integration/imageclient_test.go index e9bee8fd1701..e58a0c46f48a 100644 --- a/test/integration/imageclient_test.go +++ b/test/integration/imageclient_test.go @@ -232,11 +232,11 @@ func NewTestImageOpenShift(t *testing.T) *testImageOpenshift { interfaces, _ := latest.InterfacesFor(latest.Version) - imageEtcd := imageetcd.New(etcdHelper) + imageEtcd := imageetcd.New(etcdHelper, imageetcd.DefaultRegistryFunc(func() (string, bool) { return openshift.dockerServer.URL, true })) storage := map[string]apiserver.RESTStorage{ "images": image.NewREST(imageEtcd), - "imageRepositories": imagerepository.NewREST(imageEtcd, openshift.dockerServer.URL), + "imageRepositories": imagerepository.NewREST(imageEtcd), "imageRepositoryMappings": imagerepositorymapping.NewREST(imageEtcd, imageEtcd), "imageRepositoryTags": imagerepositorytag.NewREST(imageEtcd, imageEtcd), } From 665fa8e489f869cc679200a2783f72bc0c178586 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:26:47 -0500 Subject: [PATCH 09/18] Add "from" object reference to both ImageChangeTriggers Preserve existing behavior, but ensure that From takes precendence. --- pkg/build/api/types.go | 8 ++- pkg/build/api/v1beta1/conversion.go | 33 ++++++++++- pkg/build/api/v1beta1/conversion_test.go | 55 +++++++++++++++++-- pkg/build/api/v1beta1/types.go | 10 +++- pkg/build/api/validation/validation.go | 10 ++-- .../controller/image_change_controller.go | 2 +- .../image_change_controller_test.go | 4 +- pkg/cmd/cli/describe/describer.go | 2 +- pkg/deploy/api/types.go | 7 +++ pkg/deploy/api/v1beta1/types.go | 31 +++++++---- .../imagechange_buildtrigger_test.go | 2 +- 11 files changed, 133 insertions(+), 31 deletions(-) diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 5b9424e696c9..84b206de5f44 100644 --- a/pkg/build/api/types.go +++ b/pkg/build/api/types.go @@ -256,8 +256,12 @@ type ImageChangeTrigger struct { // Image is used to specify the value in the BuildConfig to replace with the // immutable image id supplied by the ImageRepository when this trigger fires. Image string `json:"image"` - // ImageRepositoryRef a reference to a Docker image repository to watch for changes. - ImageRepositoryRef *kapi.ObjectReference `json:"imageRepositoryRef"` + // From is a reference to a Docker image repository to watch for changes. This field takes + // precedence over ImageRepositoryRef, which is deprecated and will be removed in v1beta2. The + // Kind may be left blank, in which case it defaults to "ImageRepository". The "Name" is + // the only required subfield - if Namespace is blank, the namespace of the current deployment + // trigger will be used. + From kapi.ObjectReference `json:"from"` // Tag is the name of an image repository tag to watch for changes. Tag string `json:"tag,omitempty"` // LastTriggeredImageID is used internally by the ImageChangeController to save last diff --git a/pkg/build/api/v1beta1/conversion.go b/pkg/build/api/v1beta1/conversion.go index fa08ed18cb6b..01bd40d03c30 100644 --- a/pkg/build/api/v1beta1/conversion.go +++ b/pkg/build/api/v1beta1/conversion.go @@ -2,6 +2,7 @@ package v1beta1 import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3" "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" newer "github.com/openshift/origin/pkg/build/api" image "github.com/openshift/origin/pkg/image/api" @@ -59,5 +60,35 @@ func init() { } return nil }, - ) + // Rename ImageRepositoryRef to From + func(in *newer.ImageChangeTrigger, out *ImageChangeTrigger, s conversion.Scope) error { + if err := s.Convert(&in.From, &out.From, 0); err != nil { + return err + } + if len(in.From.Name) != 0 { + out.ImageRepositoryRef = &kapi.ObjectReference{} + if err := s.Convert(&in.From, out.ImageRepositoryRef, conversion.AllowDifferentFieldTypeNames); err != nil { + return err + } + } + out.Tag = in.Tag + out.LastTriggeredImageID = in.LastTriggeredImageID + out.Image = in.Image + return nil + }, + func(in *ImageChangeTrigger, out *newer.ImageChangeTrigger, s conversion.Scope) error { + if in.ImageRepositoryRef != nil { + if err := s.Convert(in.ImageRepositoryRef, &out.From, conversion.AllowDifferentFieldTypeNames); err != nil { + return err + } + } else { + if err := s.Convert(&in.From, &out.From, 0); err != nil { + return err + } + } + out.Tag = in.Tag + out.LastTriggeredImageID = in.LastTriggeredImageID + out.Image = in.Image + return nil + }) } diff --git a/pkg/build/api/v1beta1/conversion_test.go b/pkg/build/api/v1beta1/conversion_test.go index 4e83259e0e3c..fa068c2dcc85 100644 --- a/pkg/build/api/v1beta1/conversion_test.go +++ b/pkg/build/api/v1beta1/conversion_test.go @@ -4,6 +4,8 @@ import ( "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3" + newer "github.com/openshift/origin/pkg/build/api" current "github.com/openshift/origin/pkg/build/api/v1beta1" ) @@ -11,15 +13,60 @@ import ( var Convert = api.Scheme.Convert func TestSTIBuildStrategyConversion(t *testing.T) { - var got newer.STIBuildStrategy + var actual newer.STIBuildStrategy oldVersion := current.STIBuildStrategy{ BuilderImage: "testimage", } - err := Convert(&oldVersion, &got) + err := Convert(&oldVersion, &actual) if err != nil { t.Fatalf("unexpected error: %v", err) } - if got.Image != oldVersion.BuilderImage { - t.Error("expected %v, got %v", oldVersion.BuilderImage, got.Image) + if actual.Image != oldVersion.BuilderImage { + t.Errorf("expected %v, actual %v", oldVersion.BuilderImage, actual.Image) + } +} + +func TestImageChangeTriggerFromRename(t *testing.T) { + old := current.ImageChangeTrigger{ + From: kapi.ObjectReference{ + Name: "foo", + }, + ImageRepositoryRef: &kapi.ObjectReference{ + Name: "bar", + }, + } + actual := newer.ImageChangeTrigger{} + if err := Convert(&old, &actual); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if actual.From.Name != "bar" { + t.Error("expected %#v, actual %#v", old, actual) + } + + old = current.ImageChangeTrigger{ + From: kapi.ObjectReference{ + Name: "foo", + }, + } + actual = newer.ImageChangeTrigger{} + if err := Convert(&old, &actual); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if actual.From.Name != "foo" { + t.Error("expected %#v, actual %#v", old, actual) + } + + old = current.ImageChangeTrigger{ + From: kapi.ObjectReference{ + Name: "foo", + }, + ImageRepositoryRef: &kapi.ObjectReference{}, + } + actual = newer.ImageChangeTrigger{} + if err := Convert(&old, &actual); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if actual.From.Name != "" { + t.Errorf("expected %#v, actual %#v", old, actual) } } diff --git a/pkg/build/api/v1beta1/types.go b/pkg/build/api/v1beta1/types.go index 12e5ac2ee899..9bae38565890 100644 --- a/pkg/build/api/v1beta1/types.go +++ b/pkg/build/api/v1beta1/types.go @@ -259,8 +259,16 @@ type WebHookTrigger struct { type ImageChangeTrigger struct { // Image is used to specify the value in the BuildConfig to replace with the // immutable image id supplied by the ImageRepository when this trigger fires. - Image string `json:"image"` + Image string `json:"image"` + RepositoryName string `json:"repositoryName,omitempty"` + // From is a reference to a Docker image repository to watch for changes. This field takes + // precedence over ImageRepositoryRef, which is deprecated and will be removed in v1beta2. The + // Kind may be left blank, in which case it defaults to "ImageRepository". The "Name" is + // the only required subfield - if Namespace is blank, the namespace of the current deployment + // trigger will be used. + From kapi.ObjectReference `json:"from"` // ImageRepositoryRef a reference to a Docker image repository to watch for changes. + // DEPRECATED: replaced by From ImageRepositoryRef *kapi.ObjectReference `json:"imageRepositoryRef"` // Tag is the name of an image repository tag to watch for changes. Tag string `json:"tag,omitempty"` diff --git a/pkg/build/api/validation/validation.go b/pkg/build/api/validation/validation.go index 4737f342b627..3503f7a5c0df 100644 --- a/pkg/build/api/validation/validation.go +++ b/pkg/build/api/validation/validation.go @@ -237,12 +237,10 @@ func validateImageChange(imageChange *buildapi.ImageChangeTrigger) errs.Validati if len(imageChange.Image) == 0 { allErrs = append(allErrs, errs.NewFieldRequired("image", "")) } - if imageChange.ImageRepositoryRef == nil { - allErrs = append(allErrs, errs.NewFieldRequired("imageRepositoryRef", "")) - } else if len(imageChange.ImageRepositoryRef.Name) == 0 { - nestedErrs := errs.ValidationErrorList{errs.NewFieldRequired("name", "")} - nestedErrs.Prefix("imageRepositoryRef") - allErrs = append(allErrs, nestedErrs...) + if len(imageChange.From.Name) == 0 { + allErrs = append(allErrs, errs.NewFieldRequired("from", "")) + } else if len(imageChange.From.Name) == 0 { + allErrs = append(allErrs, errs.ValidationErrorList{errs.NewFieldRequired("name", "")}.Prefix("from")...) } return allErrs } diff --git a/pkg/build/controller/image_change_controller.go b/pkg/build/controller/image_change_controller.go index c2c14a8387a0..6c236d9db19d 100644 --- a/pkg/build/controller/image_change_controller.go +++ b/pkg/build/controller/image_change_controller.go @@ -66,7 +66,7 @@ func (c *ImageChangeController) HandleImageRepo() { } // (must be different) to trigger a build - if icTrigger.ImageRepositoryRef.Name == imageRepo.Name && icTrigger.LastTriggeredImageID != imageID { + if icTrigger.From.Name == imageRepo.Name && icTrigger.LastTriggeredImageID != imageID { imageSubstitutions[icTrigger.Image] = imageRepo.DockerImageRepository + ":" + imageID shouldTriggerBuild = true icTrigger.LastTriggeredImageID = imageID diff --git a/pkg/build/controller/image_change_controller_test.go b/pkg/build/controller/image_change_controller_test.go index f677302b3000..c3c88f310acb 100644 --- a/pkg/build/controller/image_change_controller_test.go +++ b/pkg/build/controller/image_change_controller_test.go @@ -51,7 +51,7 @@ func mockBuildConfig(baseImage, triggerImage, repoName, repoTag string) *buildap Type: buildapi.ImageChangeBuildTriggerType, ImageChange: &buildapi.ImageChangeTrigger{ Image: triggerImage, - ImageRepositoryRef: &kapi.ObjectReference{ + From: kapi.ObjectReference{ Name: repoName, }, Tag: repoTag, @@ -66,7 +66,7 @@ func appendTrigger(buildcfg *buildapi.BuildConfig, triggerImage, repoName, repoT Type: buildapi.ImageChangeBuildTriggerType, ImageChange: &buildapi.ImageChangeTrigger{ Image: triggerImage, - ImageRepositoryRef: &kapi.ObjectReference{ + From: kapi.ObjectReference{ Name: repoName, }, Tag: repoTag, diff --git a/pkg/cmd/cli/describe/describer.go b/pkg/cmd/cli/describe/describer.go index d950c6ef96d1..d04538fef606 100644 --- a/pkg/cmd/cli/describe/describer.go +++ b/pkg/cmd/cli/describe/describer.go @@ -133,7 +133,7 @@ func (d *BuildConfigDescriber) DescribeTriggers(bc *buildapi.BuildConfig, host s if trigger.Type != buildapi.ImageChangeBuildTriggerType { continue } - formatString(out, "Image Repository Trigger", trigger.ImageChange.ImageRepositoryRef.Name) + formatString(out, "Image Repository Trigger", trigger.ImageChange.From.Name) formatString(out, "- Tag", trigger.ImageChange.Tag) formatString(out, "- Image", trigger.ImageChange.Image) formatString(out, "- LastTriggeredImageID", trigger.ImageChange.LastTriggeredImageID) diff --git a/pkg/deploy/api/types.go b/pkg/deploy/api/types.go index ae4a1504b805..429ec85880cd 100644 --- a/pkg/deploy/api/types.go +++ b/pkg/deploy/api/types.go @@ -172,7 +172,14 @@ type DeploymentTriggerImageChangeParams struct { // ContainerNames is used to restrict tag updates to the specified set of container names in a pod. ContainerNames []string `json:"containerNames,omitempty"` // RepositoryName is the identifier for a Docker image repository to watch for changes. + // DEPRECATED: will be removed in v1beta2. RepositoryName string `json:"repositoryName,omitempty"` + // From is a reference to a Docker image repository to watch for changes. This field takes + // precedence over RepositoryName, which is deprecated and will be removed in v1beta2. The + // Kind may be left blank, in which case it defaults to "ImageRepository". The "Name" is + // the only required subfield - if Namespace is blank, the namespace of the current deployment + // trigger will be used. + From kapi.ObjectReference `json:"from"` // Tag is the name of an image repository tag to watch for changes. Tag string `json:"tag,omitempty"` } diff --git a/pkg/deploy/api/v1beta1/types.go b/pkg/deploy/api/v1beta1/types.go index 127996f19f7f..e71034b65c6d 100644 --- a/pkg/deploy/api/v1beta1/types.go +++ b/pkg/deploy/api/v1beta1/types.go @@ -2,7 +2,7 @@ package v1beta1 import ( v1beta1 "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1" - v1beta3 "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3" + kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta3" ) // A deployment represents a single configuration of a pod deployed into the cluster, and may @@ -11,8 +11,8 @@ import ( // DEPRECATED: This type longer drives any system behavior. Deployments are now represented directly // by ReplicationControllers. Use DeploymentConfig to drive deployments. type Deployment struct { - v1beta3.TypeMeta `json:",inline"` - v1beta3.ObjectMeta `json:"metadata,omitempty"` + kapi.TypeMeta `json:",inline"` + kapi.ObjectMeta `json:"metadata,omitempty"` // Strategy describes how a deployment is executed. Strategy DeploymentStrategy `json:"strategy,omitempty"` @@ -70,7 +70,7 @@ type CustomDeploymentStrategyParams struct { // Image specifies a Docker image which can carry out a deployment. Image string `json:"image,omitempty"` // Environment holds the environment which will be given to the container for Image. - Environment []v1beta3.EnvVar `json:"environment,omitempty"` + Environment []kapi.EnvVar `json:"environment,omitempty"` // Command is optional and overrides CMD in the container Image. Command []string `json:"command,omitempty"` } @@ -78,9 +78,9 @@ type CustomDeploymentStrategyParams struct { // A DeploymentList is a collection of deployments. // DEPRECATED: Like Deployment, this is no longer used. type DeploymentList struct { - v1beta3.TypeMeta `json:",inline"` - v1beta3.ListMeta `json:"metadata,omitempty"` - Items []Deployment `json:"items"` + kapi.TypeMeta `json:",inline"` + kapi.ListMeta `json:"metadata,omitempty"` + Items []Deployment `json:"items"` } // These constants represent keys used for correlating objects related to deployments. @@ -120,8 +120,8 @@ const ( // state of the DeploymentConfig. Each change to the DeploymentConfig which should result in // a new deployment results in an increment of LatestVersion. type DeploymentConfig struct { - v1beta3.TypeMeta `json:",inline"` - v1beta3.ObjectMeta `json:"metadata,omitempty"` + kapi.TypeMeta `json:",inline"` + kapi.ObjectMeta `json:"metadata,omitempty"` // Triggers determine how updates to a DeploymentConfig result in new deployments. If no triggers // are defined, a new deployment can only occur as a result of an explicit client update to the // DeploymentConfig with a new LatestVersion. @@ -173,7 +173,14 @@ type DeploymentTriggerImageChangeParams struct { // ContainerNames is used to restrict tag updates to the specified set of container names in a pod. ContainerNames []string `json:"containerNames,omitempty"` // RepositoryName is the identifier for a Docker image repository to watch for changes. + // DEPRECATED: will be removed in v1beta2. RepositoryName string `json:"repositoryName,omitempty"` + // From is a reference to a Docker image repository to watch for changes. This field takes + // precedence over RepositoryName, which is deprecated and will be removed in v1beta2. The + // Kind may be left blank, in which case it defaults to "ImageRepository". The "Name" is + // the only required subfield - if Namespace is blank, the namespace of the current deployment + // trigger will be used. + From kapi.ObjectReference `json:"from"` // Tag is the name of an image repository tag to watch for changes. Tag string `json:"tag,omitempty"` } @@ -203,9 +210,9 @@ type DeploymentCauseImageTrigger struct { // A DeploymentConfigList is a collection of deployment configs. type DeploymentConfigList struct { - v1beta3.TypeMeta `json:",inline"` - v1beta3.ListMeta `json:"metadata,omitempty"` - Items []DeploymentConfig `json:"items"` + kapi.TypeMeta `json:",inline"` + kapi.ListMeta `json:"metadata,omitempty"` + Items []DeploymentConfig `json:"items"` } // DeploymentConfigRollback provides the input to rollback generation. diff --git a/test/integration/imagechange_buildtrigger_test.go b/test/integration/imagechange_buildtrigger_test.go index 361a422b2478..bbf94e204a98 100644 --- a/test/integration/imagechange_buildtrigger_test.go +++ b/test/integration/imagechange_buildtrigger_test.go @@ -109,7 +109,7 @@ func imageChangeBuildConfig() *buildapi.BuildConfig { Type: buildapi.ImageChangeBuildTriggerType, ImageChange: &buildapi.ImageChangeTrigger{ Image: "registry:8080/openshift/test-image", - ImageRepositoryRef: &kapi.ObjectReference{ + From: kapi.ObjectReference{ Name: "test-image-repo", }, Tag: "latest", From 3218b81c0595c352efb44de934967c975498a240 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:28:27 -0500 Subject: [PATCH 10/18] Cleanup integration tests Use testing.Logf instead of glog, add method to verbose log etcd, ensure as many tests as possible are stopping their servers. --- test/integration/auth_proxy_test.go | 22 +++++++++++----------- test/integration/basic_auth_test.go | 4 +--- test/integration/buildcfgclient_test.go | 7 +++++++ test/integration/buildclient_test.go | 24 +++++++++++++++++++++--- test/integration/cli_get_token_test.go | 4 ++-- test/integration/deploy_trigger_test.go | 5 +++-- test/integration/imageclient_test.go | 9 ++++++--- test/integration/oauthstorage_test.go | 1 + test/integration/userclient_test.go | 1 + test/integration/utils.go | 15 +++++++++++++++ test/integration/webhookgithub_test.go | 2 ++ 11 files changed, 70 insertions(+), 24 deletions(-) diff --git a/test/integration/auth_proxy_test.go b/test/integration/auth_proxy_test.go index c2469b91440b..7be46ffdbf03 100644 --- a/test/integration/auth_proxy_test.go +++ b/test/integration/auth_proxy_test.go @@ -9,8 +9,6 @@ import ( "net/url" "testing" - "github.com/golang/glog" - kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" klatest "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" kclient "github.com/GoogleCloudPlatform/kubernetes/pkg/client" @@ -86,15 +84,17 @@ func TestFrontProxyOnAuthorize(t *testing.T) { mux := http.NewServeMux() server.Install(mux, origin.OpenShiftOAuthAPIPrefix) oauthServer := httptest.NewServer(http.Handler(mux)) - glog.Infof("oauth server is on %v\n", oauthServer.URL) + defer oauthServer.Close() + t.Logf("oauth server is on %v\n", oauthServer.URL) // set up a front proxy guarding the oauth server proxyHTTPHandler := NewBasicAuthChallenger("TestRegistryAndServer", validUsers, NewXRemoteUserProxyingHandler(oauthServer.URL)) proxyServer := httptest.NewServer(proxyHTTPHandler) - glog.Infof("proxy server is on %v\n", proxyServer.URL) + defer proxyServer.Close() + t.Logf("proxy server is on %v\n", proxyServer.URL) // need to prime clients so that we can get back a code. the client must be valid - createClient(oauthEtcd, &oauthapi.Client{ObjectMeta: kapi.ObjectMeta{Name: "test"}, Secret: "secret", RedirectURIs: []string{oauthServer.URL}}) + createClient(t, oauthEtcd, &oauthapi.Client{ObjectMeta: kapi.ObjectMeta{Name: "test"}, Secret: "secret", RedirectURIs: []string{oauthServer.URL}}) // our simple URL to get back a code. We want to go through the front proxy rawAuthorizeRequest := proxyServer.URL + origin.OpenShiftOAuthAPIPrefix + "/authorize?response_type=code&client_id=test" @@ -114,7 +114,7 @@ func TestFrontProxyOnAuthorize(t *testing.T) { // and manually handling redirects and setting our auth information every time for the front proxy redirectedUrls := make([]url.URL, 10) httpClient := http.Client{ - CheckRedirect: getRedirectMethod(&redirectedUrls), + CheckRedirect: getRedirectMethod(t, &redirectedUrls), Transport: kclient.NewBasicAuthRoundTripper("sanefarmer", "who?", http.DefaultTransport), } @@ -134,21 +134,21 @@ func TestFrontProxyOnAuthorize(t *testing.T) { if len(foundCode) == 0 { t.Errorf("Did not find code in any redirect: %v", redirectedUrls) } else { - glog.Infof("Found code %v\n", foundCode) + t.Logf("Found code %v\n", foundCode) } } -func createClient(oauthEtcd oauthclient.Registry, client *oauthapi.Client) { +func createClient(t *testing.T, oauthEtcd oauthclient.Registry, client *oauthapi.Client) { if err := oauthEtcd.CreateClient(client); err != nil { - glog.Errorf("Error creating client: %v due to %v\n", client, err) + t.Errorf("Error creating client: %v due to %v\n", client, err) } } type checkRedirect func(req *http.Request, via []*http.Request) error -func getRedirectMethod(redirectRecord *[]url.URL) checkRedirect { +func getRedirectMethod(t *testing.T, redirectRecord *[]url.URL) checkRedirect { return func(req *http.Request, via []*http.Request) error { - glog.Infof("Going to %v\n", req.URL) + t.Logf("Going to %v\n", req.URL) *redirectRecord = append(*redirectRecord, *req.URL) if len(via) >= 10 { diff --git a/test/integration/basic_auth_test.go b/test/integration/basic_auth_test.go index c66600349b30..f1eb2fc0422a 100644 --- a/test/integration/basic_auth_test.go +++ b/test/integration/basic_auth_test.go @@ -3,13 +3,11 @@ package integration import ( "log" "net/http" - - "github.com/golang/glog" ) func ExampleNewBasicAuthChallenger() { challenger := NewBasicAuthChallenger("realm", []User{{"username", "password", "Brave Butcher", "cowardly_butcher@example.org"}}, NewIdentifyingHandler()) http.Handle("/", challenger) - glog.Infoln("Auth server listening on http://localhost:1234") + log.Printf("Auth server listening on http://localhost:1234") log.Fatal(http.ListenAndServe(":1234", nil)) } diff --git a/test/integration/buildcfgclient_test.go b/test/integration/buildcfgclient_test.go index 02a62bb3dc8c..665e2e32c5b9 100644 --- a/test/integration/buildcfgclient_test.go +++ b/test/integration/buildcfgclient_test.go @@ -18,6 +18,7 @@ func init() { func TestListBuildConfigs(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfigs, err := openshift.Client.BuildConfigs(testNamespace).List(labels.Everything(), labels.Everything()) if err != nil { @@ -31,6 +32,7 @@ func TestListBuildConfigs(t *testing.T) { func TestCreateBuildConfig(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfig := mockBuildConfig() expected, err := openshift.Client.BuildConfigs(testNamespace).Create(buildConfig) @@ -53,6 +55,7 @@ func TestCreateBuildConfig(t *testing.T) { func TestUpdateBuildConfig(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfig := mockBuildConfig() actual, err := openshift.Client.BuildConfigs(testNamespace).Create(buildConfig) @@ -71,6 +74,7 @@ func TestUpdateBuildConfig(t *testing.T) { func TestDeleteBuildConfig(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfig := mockBuildConfig() actual, err := openshift.Client.BuildConfigs(testNamespace).Create(buildConfig) @@ -85,12 +89,14 @@ func TestDeleteBuildConfig(t *testing.T) { func TestWatchBuildConfigs(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfig := mockBuildConfig() watch, err := openshift.Client.BuildConfigs(testNamespace).Watch(labels.Everything(), labels.Everything(), "0") if err != nil { t.Fatalf("Unexpected error: %v", err) } + defer watch.Stop() expected, err := openshift.Client.BuildConfigs(testNamespace).Create(buildConfig) if err != nil { @@ -144,6 +150,7 @@ func mockBuildConfig() *buildapi.BuildConfig { func TestBuildConfigClient(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() buildConfigs, err := openshift.Client.BuildConfigs(testNamespace).List(labels.Everything(), labels.Everything()) if err != nil { diff --git a/test/integration/buildclient_test.go b/test/integration/buildclient_test.go index 6be1e85755ec..9485e09aff61 100644 --- a/test/integration/buildclient_test.go +++ b/test/integration/buildclient_test.go @@ -5,6 +5,7 @@ package integration import ( "net/http" "net/http/httptest" + "sync" "testing" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -15,7 +16,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/master" "github.com/GoogleCloudPlatform/kubernetes/plugin/pkg/admission/admit" - "github.com/golang/glog" "github.com/openshift/origin/pkg/api/latest" buildapi "github.com/openshift/origin/pkg/build/api" @@ -36,6 +36,7 @@ func init() { func TestListBuilds(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() builds, err := openshift.Client.Builds(testNamespace).List(labels.Everything(), labels.Everything()) if err != nil { @@ -49,6 +50,7 @@ func TestListBuilds(t *testing.T) { func TestCreateBuild(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() build := mockBuild() expected, err := openshift.Client.Builds(testNamespace).Create(build) @@ -71,6 +73,7 @@ func TestCreateBuild(t *testing.T) { func TestDeleteBuild(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() build := mockBuild() actual, err := openshift.Client.Builds(testNamespace).Create(build) @@ -85,12 +88,14 @@ func TestDeleteBuild(t *testing.T) { func TestWatchBuilds(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() build := mockBuild() watch, err := openshift.Client.Builds(testNamespace).Watch(labels.Everything(), labels.Everything(), "0") if err != nil { t.Fatalf("Unexpected error: %v", err) } + defer watch.Stop() expected, err := openshift.Client.Builds(testNamespace).Create(build) if err != nil { @@ -137,10 +142,14 @@ type testBuildOpenshift struct { Client *osclient.Client server *httptest.Server whPrefix string + stop chan struct{} + lock sync.Mutex } func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift { - openshift := &testBuildOpenshift{} + openshift := &testBuildOpenshift{ + stop: make(chan struct{}), + } etcdClient := newEtcdClient() etcdHelper, _ := master.NewEtcdHelper(etcdClient, klatest.Version) @@ -155,7 +164,7 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift { kubeletClient, err := kclient.NewKubeletClient(&kclient.KubeletConfig{Port: 10250}) if err != nil { - glog.Fatalf("Unable to configure Kubelet client: %v", err) + t.Fatalf("Unable to configure Kubelet client: %v", err) } kmaster := master.New(&master.Config{ @@ -200,6 +209,7 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift { TempDirectoryCreator: buildstrategy.STITempDirectoryCreator, UseLocalImages: false, }, + Stop: openshift.stop, } factory.Create().Run() @@ -207,6 +217,14 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift { return openshift } +func (t *testBuildOpenshift) Close() { + t.lock.Lock() + defer t.lock.Unlock() + close(t.stop) + t.server.CloseClientConnections() + t.server.Close() +} + type clientWebhookInterface struct { Client osclient.Interface } diff --git a/test/integration/cli_get_token_test.go b/test/integration/cli_get_token_test.go index ca4ecea53168..d716c0d88eaf 100644 --- a/test/integration/cli_get_token_test.go +++ b/test/integration/cli_get_token_test.go @@ -9,7 +9,6 @@ import ( "strings" "testing" - "github.com/golang/glog" "github.com/spf13/pflag" klatest "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" @@ -80,7 +79,8 @@ func TestGetToken(t *testing.T) { mux := http.NewServeMux() server.Install(mux, origin.OpenShiftOAuthAPIPrefix) oauthServer := httptest.NewServer(http.Handler(mux)) - glog.Infof("oauth server is on %v\n", oauthServer.URL) + defer oauthServer.Close() + t.Logf("oauth server is on %v\n", oauthServer.URL) // create the default oauth clients with redirects to our server origin.CreateOrUpdateDefaultOAuthClients(oauthServer.URL, oauthEtcd) diff --git a/test/integration/deploy_trigger_test.go b/test/integration/deploy_trigger_test.go index bc7086d5b512..a04ebb31ca1c 100644 --- a/test/integration/deploy_trigger_test.go +++ b/test/integration/deploy_trigger_test.go @@ -3,7 +3,6 @@ package integration import ( - "flag" "net/http" "net/http/httptest" "testing" @@ -111,6 +110,7 @@ func TestSimpleImageChangeTrigger(t *testing.T) { if err != nil { t.Fatalf("Couldn't subscribe to Deployments %v", err) } + defer watch.Stop() imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo) if err != nil { @@ -168,6 +168,7 @@ func TestSimpleConfigChangeTrigger(t *testing.T) { if err != nil { t.Fatalf("Couldn't subscribe to Deployments %v", err) } + defer watch.Stop() // submit the initial deployment config if _, err := openshift.Client.DeploymentConfigs(testNamespace).Create(config); err != nil { @@ -241,7 +242,6 @@ type testOpenshift struct { } func NewTestOpenshift(t *testing.T) *testOpenshift { - flag.Set("v", "4") t.Logf("Starting test openshift") openshift := &testOpenshift{ @@ -346,6 +346,7 @@ func NewTestOpenshift(t *testing.T) *testOpenshift { TempDirectoryCreator: buildstrategy.STITempDirectoryCreator, UseLocalImages: false, }, + Stop: openshift.stop, } bcFactory.Create().Run() diff --git a/test/integration/imageclient_test.go b/test/integration/imageclient_test.go index e58a0c46f48a..1b874e0a51b6 100644 --- a/test/integration/imageclient_test.go +++ b/test/integration/imageclient_test.go @@ -18,7 +18,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/master" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" "github.com/GoogleCloudPlatform/kubernetes/plugin/pkg/admission/admit" - "github.com/golang/glog" "github.com/openshift/origin/pkg/api/latest" osclient "github.com/openshift/origin/pkg/client" @@ -193,15 +192,19 @@ type testImageOpenshift struct { Client *osclient.Client server *httptest.Server dockerServer *httptest.Server + stop chan struct{} } func (o *testImageOpenshift) Close() { + close(o.stop) o.server.Close() o.dockerServer.Close() } func NewTestImageOpenShift(t *testing.T) *testImageOpenshift { - openshift := &testImageOpenshift{} + openshift := &testImageOpenshift{ + stop: make(chan struct{}), + } etcdClient := newEtcdClient() etcdHelper, _ := master.NewEtcdHelper(etcdClient, klatest.Version) @@ -219,7 +222,7 @@ func NewTestImageOpenShift(t *testing.T) *testImageOpenshift { kubeletClient, err := kclient.NewKubeletClient(&kclient.KubeletConfig{Port: 10250}) if err != nil { - glog.Fatalf("Unable to configure Kubelet client: %v", err) + t.Fatalf("Unable to configure Kubelet client: %v", err) } kmaster := master.New(&master.Config{ diff --git a/test/integration/oauthstorage_test.go b/test/integration/oauthstorage_test.go index 887959f4e01a..25b1aa017041 100644 --- a/test/integration/oauthstorage_test.go +++ b/test/integration/oauthstorage_test.go @@ -80,6 +80,7 @@ func TestOAuthStorage(t *testing.T) { mux := http.NewServeMux() oauthServer.Install(mux, "") server := httptest.NewServer(mux) + defer server.Close() ch := make(chan *osincli.AccessData, 1) var oaclient *osincli.Client diff --git a/test/integration/userclient_test.go b/test/integration/userclient_test.go index 32ac9eb92f04..cfed45470edd 100644 --- a/test/integration/userclient_test.go +++ b/test/integration/userclient_test.go @@ -45,6 +45,7 @@ func TestUserInitialization(t *testing.T) { } server := httptest.NewServer(apiserver.Handle(storage, v1beta1.Codec, "/osapi", "v1beta1", interfaces.MetadataAccessor, admit.NewAlwaysAdmit())) + defer server.Close() mapping := api.UserIdentityMapping{ Identity: api.Identity{ diff --git a/test/integration/utils.go b/test/integration/utils.go index 25112100626c..0c49c8ac65ba 100644 --- a/test/integration/utils.go +++ b/test/integration/utils.go @@ -3,12 +3,16 @@ package integration import ( + "flag" "fmt" + "log" "math/rand" "os" "github.com/coreos/go-etcd/etcd" "github.com/golang/glog" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/capabilities" ) const testNamespace = "integration-test" @@ -24,6 +28,17 @@ func newEtcdClient() *etcd.Client { return etcd.NewClient(etcdServers) } +func init() { + capabilities.SetForTests(capabilities.Capabilities{ + AllowPrivileged: true, + }) + flag.Set("v", "5") +} + +func logEtcd() { + etcd.SetLogger(log.New(os.Stderr, "go-etcd", log.LstdFlags)) +} + func requireEtcd() { if _, err := newEtcdClient().Get("/", false, false); err != nil { glog.Fatalf("unable to connect to etcd for integration testing: %v", err) diff --git a/test/integration/webhookgithub_test.go b/test/integration/webhookgithub_test.go index 8b2204a0f1bf..8460cdf274c8 100644 --- a/test/integration/webhookgithub_test.go +++ b/test/integration/webhookgithub_test.go @@ -22,6 +22,7 @@ func init() { func TestWebhookGithubPush(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() // create buildconfig buildConfig := &buildapi.BuildConfig{ @@ -78,6 +79,7 @@ func TestWebhookGithubPush(t *testing.T) { func TestWebhookGithubPing(t *testing.T) { deleteAllEtcdKeys() openshift := NewTestBuildOpenshift(t) + defer openshift.Close() // create buildconfig buildConfig := &buildapi.BuildConfig{ From 132c5b760c161940656fce6cc1e332860dd21a37 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:30:19 -0500 Subject: [PATCH 11/18] Return the most recently updated deployment config after PUT --- pkg/deploy/registry/deployconfig/rest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/deploy/registry/deployconfig/rest.go b/pkg/deploy/registry/deployconfig/rest.go index 287fa1544d90..4be79ea453e8 100644 --- a/pkg/deploy/registry/deployconfig/rest.go +++ b/pkg/deploy/registry/deployconfig/rest.go @@ -118,6 +118,6 @@ func (s *REST) Update(ctx kapi.Context, obj runtime.Object) (<-chan apiserver.RE if err != nil { return nil, err } - return deploymentConfig, nil + return s.Get(ctx, deploymentConfig.Name) }), nil } From 63abc80e891d0269e45944b12135fd552766ee90 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:31:31 -0500 Subject: [PATCH 12/18] Allow more goroutines to exit during test cases with RunUntil --- pkg/build/controller/factory/factory.go | 20 +++++++++++++------- pkg/cmd/server/etcd/etcd.go | 4 +++- pkg/deploy/controller/factory/factory.go | 16 ++++++++-------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/pkg/build/controller/factory/factory.go b/pkg/build/controller/factory/factory.go index 0a790bd69b57..602d68831236 100644 --- a/pkg/build/controller/factory/factory.go +++ b/pkg/build/controller/factory/factory.go @@ -27,16 +27,18 @@ type BuildControllerFactory struct { DockerBuildStrategy *strategy.DockerBuildStrategy STIBuildStrategy *strategy.STIBuildStrategy CustomBuildStrategy *strategy.CustomBuildStrategy + // Stop may be set to allow controllers created by this factory to be terminated. + Stop <-chan struct{} buildStore cache.Store } func (factory *BuildControllerFactory) Create() *controller.BuildController { factory.buildStore = cache.NewStore() - cache.NewReflector(&buildLW{client: factory.Client}, &buildapi.Build{}, factory.buildStore).Run() + cache.NewReflector(&buildLW{client: factory.Client}, &buildapi.Build{}, factory.buildStore).RunUntil(factory.Stop) buildQueue := cache.NewFIFO() - cache.NewReflector(&buildLW{client: factory.Client}, &buildapi.Build{}, buildQueue).Run() + cache.NewReflector(&buildLW{client: factory.Client}, &buildapi.Build{}, buildQueue).RunUntil(factory.Stop) // Kubernetes does not currently synchronize Pod status in storage with a Pod's container // states. Because of this, we can't receive events related to container (and thus Pod) @@ -47,17 +49,21 @@ func (factory *BuildControllerFactory) Create() *controller.BuildController { // TODO: Find a way to get watch events for Pod/container status updates. The polling // strategy is horribly inefficient and should be addressed upstream somehow. podQueue := cache.NewFIFO() - cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).Run() + cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).RunUntil(factory.Stop) return &controller.BuildController{ BuildStore: factory.buildStore, BuildUpdater: ClientBuildUpdater{factory.Client}, PodManager: ClientPodManager{factory.KubeClient}, NextBuild: func() *buildapi.Build { - return buildQueue.Pop().(*buildapi.Build) + build := buildQueue.Pop().(*buildapi.Build) + panicIfStopped(factory.Stop, "build controller stopped") + return build }, NextPod: func() *kapi.Pod { - return podQueue.Pop().(*kapi.Pod) + pod := podQueue.Pop().(*kapi.Pod) + panicIfStopped(factory.Stop, "build controller stopped") + return pod }, BuildStrategy: &typeBasedFactoryStrategy{ DockerBuildStrategy: factory.DockerBuildStrategy, @@ -79,10 +85,10 @@ type ImageChangeControllerFactory struct { // image is available func (factory *ImageChangeControllerFactory) Create() *controller.ImageChangeController { queue := cache.NewFIFO() - cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).Run() + cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).RunUntil(factory.Stop) store := cache.NewStore() - cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).Run() + cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).RunUntil(factory.Stop) return &controller.ImageChangeController{ BuildConfigStore: store, diff --git a/pkg/cmd/server/etcd/etcd.go b/pkg/cmd/server/etcd/etcd.go index b8f13a870c0e..9d98f1114000 100644 --- a/pkg/cmd/server/etcd/etcd.go +++ b/pkg/cmd/server/etcd/etcd.go @@ -1,6 +1,8 @@ package etcd import ( + "time" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" etcdconfig "github.com/coreos/etcd/config" "github.com/coreos/etcd/etcd" @@ -29,6 +31,6 @@ func (c *Config) Run() { glog.Infof("Started etcd at %s", config.Addr) server.Run() glog.Fatalf("etcd died, exiting.") - }, 0) + }, 500*time.Millisecond) <-server.ReadyNotify() } diff --git a/pkg/deploy/controller/factory/factory.go b/pkg/deploy/controller/factory/factory.go index 3e923e32f504..fd7a4b1571fe 100644 --- a/pkg/deploy/controller/factory/factory.go +++ b/pkg/deploy/controller/factory/factory.go @@ -29,7 +29,7 @@ type DeploymentConfigControllerFactory struct { func (factory *DeploymentConfigControllerFactory) Create() *controller.DeploymentConfigController { queue := cache.NewFIFO() - cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, queue).Run() + cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, queue).RunUntil(factory.Stop) return &controller.DeploymentConfigController{ DeploymentInterface: &ClientDeploymentInterface{factory.KubeClient}, @@ -68,10 +68,10 @@ type DeploymentControllerFactory struct { func (factory *DeploymentControllerFactory) Create() *controller.DeploymentController { deploymentQueue := cache.NewFIFO() - cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, deploymentQueue).Run() + cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, deploymentQueue).RunUntil(factory.Stop) factory.deploymentStore = cache.NewStore() - cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, factory.deploymentStore).Run() + cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, factory.deploymentStore).RunUntil(factory.Stop) // Kubernetes does not currently synchronize Pod status in storage with a Pod's container // states. Because of this, we can't receive events related to container (and thus Pod) @@ -82,7 +82,7 @@ func (factory *DeploymentControllerFactory) Create() *controller.DeploymentContr // TODO: Find a way to get watch events for Pod/container status updates. The polling // strategy is horribly inefficient and should be addressed upstream somehow. podQueue := cache.NewFIFO() - cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).Run() + cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).RunUntil(factory.Stop) return &controller.DeploymentController{ ContainerCreator: factory, @@ -201,10 +201,10 @@ type DeploymentConfigChangeControllerFactory struct { func (factory *DeploymentConfigChangeControllerFactory) Create() *controller.DeploymentConfigChangeController { queue := cache.NewFIFO() - cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, queue).Run() + cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, queue).RunUntil(factory.Stop) store := cache.NewStore() - cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, store).Run() + cache.NewReflector(&deploymentLW{client: factory.KubeClient, field: labels.Everything()}, &kapi.ReplicationController{}, store).RunUntil(factory.Stop) return &controller.DeploymentConfigChangeController{ ChangeStrategy: &ClientDeploymentConfigInterface{factory.Client}, @@ -229,10 +229,10 @@ type ImageChangeControllerFactory struct { func (factory *ImageChangeControllerFactory) Create() *controller.ImageChangeController { queue := cache.NewFIFO() - cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).Run() + cache.NewReflector(&imageRepositoryLW{factory.Client}, &imageapi.ImageRepository{}, queue).RunUntil(factory.Stop) store := cache.NewStore() - cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, store).Run() + cache.NewReflector(&deploymentConfigLW{factory.Client}, &deployapi.DeploymentConfig{}, store).RunUntil(factory.Stop) return &controller.ImageChangeController{ DeploymentConfigInterface: &ClientDeploymentConfigInterface{factory.Client}, From 9a204c157169a9f4279b0815eb7fbca638da55d7 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 17:32:01 -0500 Subject: [PATCH 13/18] Remove the need to load deployments from config_generator General cleanup of some code paths, add better error handling and document things that still need to change. Significant refactor to config_generator to avoid calling ListImageRepositories as much as possible. Use the new "from" object reference in preference to "repository" field. Deployment config logic is still a bit fragile - we need to come back and have the discussion on the proposed changes to behavior here later. Also, need more tests. --- examples/jenkins/jenkins-config.json | 2 +- pkg/build/controller/factory/factory.go | 50 ++-- .../controller/image_change_controller.go | 8 +- pkg/build/registry/buildlog/rest.go | 2 + pkg/cmd/server/origin/master.go | 44 ++- pkg/deploy/api/v1beta1/types.go | 4 +- pkg/deploy/api/validation/validation.go | 101 +++++-- pkg/deploy/api/validation/validation_test.go | 74 ++++- .../controller/config_change_controller.go | 21 +- .../deployment_config_controller.go | 2 +- pkg/deploy/generator/config_generator.go | 267 ++++++++++++------ pkg/deploy/generator/config_generator_test.go | 261 ++++++++++++++--- pkg/deploy/rollback/rest.go | 64 ++--- pkg/deploy/rollback/rest_test.go | 100 ++----- pkg/deploy/rollback/rollback_generator.go | 5 +- .../rollback/rollback_generator_test.go | 2 +- pkg/deploy/util/util.go | 61 +--- test/integration/buildclient_test.go | 2 + test/integration/deploy_trigger_test.go | 88 +++++- 19 files changed, 740 insertions(+), 418 deletions(-) diff --git a/examples/jenkins/jenkins-config.json b/examples/jenkins/jenkins-config.json index 4a2db1e06713..be2c84960c4f 100644 --- a/examples/jenkins/jenkins-config.json +++ b/examples/jenkins/jenkins-config.json @@ -19,7 +19,7 @@ }, { "metadata": { - "id":"jenkins" + "name":"jenkins" }, "kind":"DeploymentConfig", "apiVersion":"v1beta1", diff --git a/pkg/build/controller/factory/factory.go b/pkg/build/controller/factory/factory.go index 602d68831236..f6af56a9ff65 100644 --- a/pkg/build/controller/factory/factory.go +++ b/pkg/build/controller/factory/factory.go @@ -51,10 +51,12 @@ func (factory *BuildControllerFactory) Create() *controller.BuildController { podQueue := cache.NewFIFO() cache.NewPoller(factory.pollPods, 10*time.Second, podQueue).RunUntil(factory.Stop) + client := ControllerClient{factory.KubeClient, factory.Client} return &controller.BuildController{ - BuildStore: factory.buildStore, - BuildUpdater: ClientBuildUpdater{factory.Client}, - PodManager: ClientPodManager{factory.KubeClient}, + BuildStore: factory.buildStore, + BuildUpdater: client, + ImageRepositoryClient: client, + PodManager: client, NextBuild: func() *buildapi.Build { build := buildQueue.Pop().(*buildapi.Build) panicIfStopped(factory.Stop, "build controller stopped") @@ -90,10 +92,11 @@ func (factory *ImageChangeControllerFactory) Create() *controller.ImageChangeCon store := cache.NewStore() cache.NewReflector(&buildConfigLW{client: factory.Client}, &buildapi.BuildConfig{}, store).RunUntil(factory.Stop) + client := ControllerClient{nil, factory.Client} return &controller.ImageChangeController{ BuildConfigStore: store, - BuildConfigUpdater: &ClientBuildConfigUpdater{factory.Client}, - BuildCreator: &ClientBuildCreator{factory.Client}, + BuildConfigUpdater: client, + BuildCreator: client, NextImageRepository: func() *imageapi.ImageRepository { repo := queue.Pop().(*imageapi.ImageRepository) panicIfStopped(factory.Stop, "build image change controller stopped") @@ -216,38 +219,29 @@ func (lw *imageRepositoryLW) Watch(resourceVersion string) (watch.Interface, err return lw.client.ImageRepositories(kapi.NamespaceAll).Watch(labels.Everything(), labels.Everything(), resourceVersion) } -// ClientPodManager is a PodManager which delegates to the Kubernetes client interface. -type ClientPodManager struct { +// ControllerClient implements the common interfaces needed for build controllers +type ControllerClient struct { KubeClient kclient.Interface + Client osclient.Interface } // CreatePod creates a pod using the Kubernetes client. -func (c ClientPodManager) CreatePod(namespace string, pod *kapi.Pod) (*kapi.Pod, error) { +func (c ControllerClient) CreatePod(namespace string, pod *kapi.Pod) (*kapi.Pod, error) { return c.KubeClient.Pods(namespace).Create(pod) } // DeletePod destroys a pod using the Kubernetes client. -func (c ClientPodManager) DeletePod(namespace string, pod *kapi.Pod) error { +func (c ControllerClient) DeletePod(namespace string, pod *kapi.Pod) error { return c.KubeClient.Pods(namespace).Delete(pod.Name) } -// ClientBuildUpdater is a buildUpdater which delegates to the OpenShift client interfaces. -type ClientBuildUpdater struct { - Client osclient.Interface -} - // UpdateBuild updates build using the OpenShift client. -func (c ClientBuildUpdater) UpdateBuild(namespace string, build *buildapi.Build) (*buildapi.Build, error) { +func (c ControllerClient) UpdateBuild(namespace string, build *buildapi.Build) (*buildapi.Build, error) { return c.Client.Builds(namespace).Update(build) } -// ClientBuildCreator is a buildCreator which delegates to the OpenShift client interfaces. -type ClientBuildCreator struct { - Client osclient.Interface -} - -// UpdateBuild updates build using the OpenShift client. -func (c *ClientBuildCreator) CreateBuild(config *buildapi.BuildConfig, imageSubstitutions map[string]string) error { +// CreateBuild updates build using the OpenShift client. +func (c ControllerClient) CreateBuild(config *buildapi.BuildConfig, imageSubstitutions map[string]string) error { build := buildutil.GenerateBuildFromConfig(config, nil) for originalImage, newImage := range imageSubstitutions { buildutil.SubstituteImageReferences(build, originalImage, newImage) @@ -259,13 +253,13 @@ func (c *ClientBuildCreator) CreateBuild(config *buildapi.BuildConfig, imageSubs return nil } -// ClientBuildConfigUpdater is a buildConfigUpdater which delegates to the OpenShift client interfaces. -type ClientBuildConfigUpdater struct { - Client osclient.Interface -} - // UpdateBuildConfig updates buildConfig using the OpenShift client. -func (c *ClientBuildConfigUpdater) UpdateBuildConfig(buildConfig *buildapi.BuildConfig) error { +func (c ControllerClient) UpdateBuildConfig(buildConfig *buildapi.BuildConfig) error { _, err := c.Client.BuildConfigs(buildConfig.Namespace).Update(buildConfig) return err } + +// GetImageRepository retrieves an image repository by namespace and name +func (c ControllerClient) GetImageRepository(namespace, name string) (*imageapi.ImageRepository, error) { + return c.Client.ImageRepositories(namespace).Get(name) +} diff --git a/pkg/build/controller/image_change_controller.go b/pkg/build/controller/image_change_controller.go index 6c236d9db19d..6576de32ae12 100644 --- a/pkg/build/controller/image_change_controller.go +++ b/pkg/build/controller/image_change_controller.go @@ -42,6 +42,7 @@ func (c *ImageChangeController) HandleImageRepo() { glog.V(4).Infof("Build image change controller detected imagerepo change %s", imageRepo.DockerImageRepository) imageSubstitutions := make(map[string]string) + // TODO: this is inefficient for _, bc := range c.BuildConfigStore.List() { config := bc.(*buildapi.BuildConfig) glog.V(4).Infof("Detecting changed images for buildConfig %s", config.Name) @@ -52,10 +53,13 @@ func (c *ImageChangeController) HandleImageRepo() { if trigger.Type != buildapi.ImageChangeBuildTriggerType { continue } + icTrigger := trigger.ImageChange + if icTrigger.From.Name != imageRepo.Name { + continue + } // for every ImageChange trigger, record the image it substitutes for and get the latest // image id from the imagerepository. We will substitute all images in the buildconfig // with the latest values from the imagerepositories. - icTrigger := trigger.ImageChange tag := icTrigger.Tag if len(tag) == 0 { tag = buildapi.DefaultImageTag @@ -66,7 +70,7 @@ func (c *ImageChangeController) HandleImageRepo() { } // (must be different) to trigger a build - if icTrigger.From.Name == imageRepo.Name && icTrigger.LastTriggeredImageID != imageID { + if icTrigger.LastTriggeredImageID != imageID { imageSubstitutions[icTrigger.Image] = imageRepo.DockerImageRepository + ":" + imageID shouldTriggerBuild = true icTrigger.LastTriggeredImageID = imageID diff --git a/pkg/build/registry/buildlog/rest.go b/pkg/build/registry/buildlog/rest.go index 466108906cf3..ba8fefbbe50b 100644 --- a/pkg/build/registry/buildlog/rest.go +++ b/pkg/build/registry/buildlog/rest.go @@ -51,6 +51,8 @@ func (r *REST) ResourceLocation(ctx kapi.Context, id string) (string, error) { return "", errors.NewFieldNotFound("Build", id) } + // TODO: these must be status errors, not field errors + // TODO: choose a more appropriate "try again later" status code, like 202 if len(build.PodName) == 0 { return "", errors.NewFieldRequired("Build.PodName", build.PodName) } diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index 632185fa8c41..8c479c87e599 100644 --- a/pkg/cmd/server/origin/master.go +++ b/pkg/cmd/server/origin/master.go @@ -44,7 +44,6 @@ import ( osclient "github.com/openshift/origin/pkg/client" cmdutil "github.com/openshift/origin/pkg/cmd/util" "github.com/openshift/origin/pkg/cmd/util/clientcmd" - deployapi "github.com/openshift/origin/pkg/deploy/api" deploycontrollerfactory "github.com/openshift/origin/pkg/deploy/controller/factory" deployconfiggenerator "github.com/openshift/origin/pkg/deploy/generator" deployregistry "github.com/openshift/origin/pkg/deploy/registry/deploy" @@ -207,17 +206,22 @@ func (c *MasterConfig) InstallAPI(container *restful.Container) []string { userEtcd := useretcd.New(c.EtcdHelper, user.NewDefaultUserInitStrategy()) oauthEtcd := oauthetcd.New(c.EtcdHelper) - osclient, kclient := c.DeploymentConfigControllerClients() + // TODO: with sharding, this needs to be changed deployConfigGenerator := &deployconfiggenerator.DeploymentConfigGenerator{ - DeploymentInterface: &oldClientDeploymentInterface{kclient}, - DeploymentConfigInterface: deployEtcd, - ImageRepositoryInterface: imageEtcd, + Client: deployconfiggenerator.Client{ + DCFn: deployEtcd.GetDeploymentConfig, + IRFn: imageEtcd.GetImageRepository, + LIRFn2: imageEtcd.ListImageRepositories, + }, Codec: latest.Codec, } - - deployRollbackGenerator := &deployrollback.RollbackGenerator{} - rollbackDeploymentGetter := &clientDeploymentInterface{kclient} - rollbackDeploymentConfigGetter := &clientDeploymentConfigInterface{osclient} + _, kclient := c.DeploymentConfigControllerClients() + deployRollback := &deployrollback.RollbackGenerator{} + deployRollbackClient := deployrollback.Client{ + DCFn: deployEtcd.GetDeploymentConfig, + RCFn: clientDeploymentInterface{kclient}.GetDeployment, + GRFn: deployRollback.GenerateRollback, + } // initialize OpenShift API storage := map[string]apiserver.RESTStorage{ @@ -233,7 +237,7 @@ func (c *MasterConfig) InstallAPI(container *restful.Container) []string { "deployments": deployregistry.NewREST(deployEtcd), "deploymentConfigs": deployconfigregistry.NewREST(deployEtcd), "generateDeploymentConfigs": deployconfiggenerator.NewREST(deployConfigGenerator, v1beta1.Codec), - "deploymentConfigRollbacks": deployrollback.NewREST(deployRollbackGenerator, rollbackDeploymentGetter, rollbackDeploymentConfigGetter, latest.Codec), + "deploymentConfigRollbacks": deployrollback.NewREST(deployRollbackClient, latest.Codec), "templateConfigs": templateregistry.NewREST(), @@ -575,26 +579,10 @@ func (c ClientWebhookInterface) GetBuildConfig(namespace, name string) (*buildap return c.Client.BuildConfigs(namespace).Get(name) } -type oldClientDeploymentInterface struct { - KubeClient kclient.Interface -} - -func (c *oldClientDeploymentInterface) GetDeployment(ctx api.Context, name string) (*api.ReplicationController, error) { - return c.KubeClient.ReplicationControllers(api.Namespace(ctx)).Get(name) -} - type clientDeploymentInterface struct { KubeClient kclient.Interface } -func (c *clientDeploymentInterface) GetDeployment(namespace, name string) (*api.ReplicationController, error) { - return c.KubeClient.ReplicationControllers(namespace).Get(name) -} - -type clientDeploymentConfigInterface struct { - Client osclient.Interface -} - -func (c *clientDeploymentConfigInterface) GetDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error) { - return c.Client.DeploymentConfigs(namespace).Get(name) +func (c clientDeploymentInterface) GetDeployment(ctx api.Context, name string) (*api.ReplicationController, error) { + return c.KubeClient.ReplicationControllers(api.Namespace(ctx)).Get(name) } diff --git a/pkg/deploy/api/v1beta1/types.go b/pkg/deploy/api/v1beta1/types.go index e71034b65c6d..e38999b9e949 100644 --- a/pkg/deploy/api/v1beta1/types.go +++ b/pkg/deploy/api/v1beta1/types.go @@ -217,7 +217,7 @@ type DeploymentConfigList struct { // DeploymentConfigRollback provides the input to rollback generation. type DeploymentConfigRollback struct { - v1beta3.TypeMeta `json:",inline"` + kapi.TypeMeta `json:",inline"` // Spec defines the options to rollback generation. Spec DeploymentConfigRollbackSpec `json:"spec"` } @@ -225,7 +225,7 @@ type DeploymentConfigRollback struct { // DeploymentConfigRollbackSpec represents the options for rollback generation. type DeploymentConfigRollbackSpec struct { // From points to a ReplicationController which is a deployment. - From v1beta3.ObjectReference `json:"from"` + From kapi.ObjectReference `json:"from"` // IncludeTriggers specifies whether to include config Triggers. IncludeTriggers bool `json:"includeTriggers` // IncludeTemplate specifies whether to include the PodTemplateSpec. diff --git a/pkg/deploy/api/validation/validation.go b/pkg/deploy/api/validation/validation.go index 32a2200815ae..38b329d10d47 100644 --- a/pkg/deploy/api/validation/validation.go +++ b/pkg/deploy/api/validation/validation.go @@ -3,6 +3,8 @@ package validation import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/validation" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + deployapi "github.com/openshift/origin/pkg/deploy/api" ) @@ -11,25 +13,42 @@ import ( // upstream and fix when it goes in. func ValidateDeployment(deployment *deployapi.Deployment) errors.ValidationErrorList { - result := validateDeploymentStrategy(&deployment.Strategy).Prefix("strategy") - controllerStateErrors := validation.ValidateReplicationControllerSpec(&deployment.ControllerTemplate) - result = append(result, controllerStateErrors.Prefix("controllerTemplate")...) - - return result + errs := validateDeploymentStrategy(&deployment.Strategy).Prefix("strategy") + if len(deployment.Name) == 0 { + errs = append(errs, errors.NewFieldRequired("name", deployment.Name)) + } else if !util.IsDNSSubdomain(deployment.Name) { + errs = append(errs, errors.NewFieldInvalid("name", deployment.Name, "name must be a valid subdomain")) + } + if len(deployment.Namespace) == 0 { + errs = append(errs, errors.NewFieldRequired("namespace", deployment.Namespace)) + } else if !util.IsDNSSubdomain(deployment.Namespace) { + errs = append(errs, errors.NewFieldInvalid("namespace", deployment.Namespace, "namespace must be a valid subdomain")) + } + errs = append(errs, validation.ValidateLabels(deployment.Labels, "labels")...) + errs = append(errs, validation.ValidateReplicationControllerSpec(&deployment.ControllerTemplate).Prefix("controllerTemplate")...) + return errs } func ValidateDeploymentConfig(config *deployapi.DeploymentConfig) errors.ValidationErrorList { - result := errors.ValidationErrorList{} + errs := errors.ValidationErrorList{} + if len(config.Name) == 0 { + errs = append(errs, errors.NewFieldRequired("name", config.Name)) + } else if !util.IsDNSSubdomain(config.Name) { + errs = append(errs, errors.NewFieldInvalid("name", config.Name, "name must be a valid subdomain")) + } + if len(config.Namespace) == 0 { + errs = append(errs, errors.NewFieldRequired("namespace", config.Namespace)) + } else if !util.IsDNSSubdomain(config.Namespace) { + errs = append(errs, errors.NewFieldInvalid("namespace", config.Namespace, "namespace must be a valid subdomain")) + } + errs = append(errs, validation.ValidateLabels(config.Labels, "labels")...) for i := range config.Triggers { - result = append(result, validateTrigger(&config.Triggers[i]).PrefixIndex(i).Prefix("triggers")...) + errs = append(errs, validateTrigger(&config.Triggers[i]).PrefixIndex(i).Prefix("triggers")...) } - - result = append(result, validateDeploymentStrategy(&config.Template.Strategy).Prefix("template.strategy")...) - controllerStateErrors := validation.ValidateReplicationControllerSpec(&config.Template.ControllerTemplate) - result = append(result, controllerStateErrors.Prefix("template.controllerTemplate")...) - - return result + errs = append(errs, validateDeploymentStrategy(&config.Template.Strategy).Prefix("template.strategy")...) + errs = append(errs, validation.ValidateReplicationControllerSpec(&config.Template.ControllerTemplate).Prefix("template.controllerTemplate")...) + return errs } func ValidateDeploymentConfigRollback(rollback *deployapi.DeploymentConfigRollback) errors.ValidationErrorList { @@ -51,62 +70,82 @@ func ValidateDeploymentConfigRollback(rollback *deployapi.DeploymentConfigRollba } func validateDeploymentStrategy(strategy *deployapi.DeploymentStrategy) errors.ValidationErrorList { - result := errors.ValidationErrorList{} + errs := errors.ValidationErrorList{} if len(strategy.Type) == 0 { - result = append(result, errors.NewFieldRequired("type", "")) + errs = append(errs, errors.NewFieldRequired("type", "")) } switch strategy.Type { case deployapi.DeploymentStrategyTypeCustom: if strategy.CustomParams == nil { - result = append(result, errors.NewFieldRequired("customParams", "")) + errs = append(errs, errors.NewFieldRequired("customParams", "")) } else { - result = append(result, validateCustomParams(strategy.CustomParams).Prefix("customParams")...) + errs = append(errs, validateCustomParams(strategy.CustomParams).Prefix("customParams")...) } } - return result + return errs } func validateCustomParams(params *deployapi.CustomDeploymentStrategyParams) errors.ValidationErrorList { - result := errors.ValidationErrorList{} + errs := errors.ValidationErrorList{} if len(params.Image) == 0 { - result = append(result, errors.NewFieldRequired("image", "")) + errs = append(errs, errors.NewFieldRequired("image", "")) } - return result + return errs } func validateTrigger(trigger *deployapi.DeploymentTriggerPolicy) errors.ValidationErrorList { - result := errors.ValidationErrorList{} + errs := errors.ValidationErrorList{} if len(trigger.Type) == 0 { - result = append(result, errors.NewFieldRequired("type", "")) + errs = append(errs, errors.NewFieldRequired("type", "")) } if trigger.Type == deployapi.DeploymentTriggerOnImageChange { if trigger.ImageChangeParams == nil { - result = append(result, errors.NewFieldRequired("imageChangeParams", nil)) + errs = append(errs, errors.NewFieldRequired("imageChangeParams", nil)) } else { - result = append(result, validateImageChangeParams(trigger.ImageChangeParams).Prefix("imageChangeParams")...) + errs = append(errs, validateImageChangeParams(trigger.ImageChangeParams).Prefix("imageChangeParams")...) } } - return result + return errs } func validateImageChangeParams(params *deployapi.DeploymentTriggerImageChangeParams) errors.ValidationErrorList { - result := errors.ValidationErrorList{} + errs := errors.ValidationErrorList{} + + if len(params.From.Name) != 0 { + if len(params.From.Kind) == 0 { + params.From.Kind = "ImageRepository" + } + if params.From.Kind != "ImageRepository" { + errs = append(errs, errors.NewFieldInvalid("from.kind", params.From.Kind, "only 'ImageRepository' is allowed")) + } - if len(params.RepositoryName) == 0 { - result = append(result, errors.NewFieldRequired("repositoryName", "")) + if !util.IsDNSSubdomain(params.From.Name) { + errs = append(errs, errors.NewFieldInvalid("from.name", params.From.Name, "name must be a valid subdomain")) + } + if len(params.From.Namespace) != 0 && !util.IsDNSSubdomain(params.From.Namespace) { + errs = append(errs, errors.NewFieldInvalid("from.namespace", params.From.Namespace, "namespace must be a valid subdomain")) + } + + if len(params.RepositoryName) != 0 { + errs = append(errs, errors.NewFieldInvalid("repositoryName", params.RepositoryName, "only one of 'from', 'repository' name may be specified")) + } + } else { + if len(params.RepositoryName) == 0 { + errs = append(errs, errors.NewFieldRequired("from", "")) + } } if len(params.ContainerNames) == 0 { - result = append(result, errors.NewFieldRequired("containerNames", "")) + errs = append(errs, errors.NewFieldRequired("containerNames", "")) } - return result + return errs } diff --git a/pkg/deploy/api/validation/validation_test.go b/pkg/deploy/api/validation/validation_test.go index 3c0809491042..ebbfa3b1cd6d 100644 --- a/pkg/deploy/api/validation/validation_test.go +++ b/pkg/deploy/api/validation/validation_test.go @@ -24,6 +24,7 @@ func manualTrigger() []api.DeploymentTriggerPolicy { func TestValidateDeploymentOK(t *testing.T) { errs := ValidateDeployment(&api.Deployment{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, Strategy: test.OkStrategy(), ControllerTemplate: test.OkControllerTemplate(), }) @@ -40,6 +41,7 @@ func TestValidateDeploymentMissingFields(t *testing.T) { }{ "missing strategy.type": { api.Deployment{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, Strategy: api.DeploymentStrategy{}, ControllerTemplate: test.OkControllerTemplate(), }, @@ -66,8 +68,9 @@ func TestValidateDeploymentMissingFields(t *testing.T) { func TestValidateDeploymentConfigOK(t *testing.T) { errs := ValidateDeploymentConfig(&api.DeploymentConfig{ - Triggers: manualTrigger(), - Template: test.OkDeploymentTemplate(), + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, + Triggers: manualTrigger(), + Template: test.OkDeploymentTemplate(), }) if len(errs) > 0 { @@ -81,8 +84,42 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { T errors.ValidationErrorType F string }{ + "missing name": { + api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "", Namespace: "bar"}, + Template: test.OkDeploymentTemplate(), + }, + errors.ValidationErrorTypeRequired, + "name", + }, + "missing namespace": { + api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: ""}, + Template: test.OkDeploymentTemplate(), + }, + errors.ValidationErrorTypeRequired, + "namespace", + }, + "invalid name": { + api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "-foo", Namespace: "bar"}, + Template: test.OkDeploymentTemplate(), + }, + errors.ValidationErrorTypeInvalid, + "name", + }, + "invalid namespace": { + api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "-bar"}, + Template: test.OkDeploymentTemplate(), + }, + errors.ValidationErrorTypeInvalid, + "namespace", + }, + "missing trigger.type": { api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, Triggers: []api.DeploymentTriggerPolicy{ { ImageChangeParams: &api.DeploymentTriggerImageChangeParams{ @@ -95,8 +132,9 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { errors.ValidationErrorTypeRequired, "triggers[0].type", }, - "missing Trigger imageChangeParams.repositoryName": { + "missing Trigger imageChangeParams.from": { api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, Triggers: []api.DeploymentTriggerPolicy{ { Type: api.DeploymentTriggerOnImageChange, @@ -108,10 +146,31 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { Template: test.OkDeploymentTemplate(), }, errors.ValidationErrorTypeRequired, + "triggers[0].imageChangeParams.from", + }, + "both fields illegal Trigger imageChangeParams.repositoryName": { + api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, + Triggers: []api.DeploymentTriggerPolicy{ + { + Type: api.DeploymentTriggerOnImageChange, + ImageChangeParams: &api.DeploymentTriggerImageChangeParams{ + ContainerNames: []string{"foo"}, + RepositoryName: "name", + From: kapi.ObjectReference{ + Name: "other", + }, + }, + }, + }, + Template: test.OkDeploymentTemplate(), + }, + errors.ValidationErrorTypeInvalid, "triggers[0].imageChangeParams.repositoryName", }, "missing Trigger imageChangeParams.containerNames": { api.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, Triggers: []api.DeploymentTriggerPolicy{ { Type: api.DeploymentTriggerOnImageChange, @@ -127,7 +186,8 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { }, "missing strategy.type": { api.DeploymentConfig{ - Triggers: manualTrigger(), + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, + Triggers: manualTrigger(), Template: api.DeploymentTemplate{ Strategy: api.DeploymentStrategy{ CustomParams: test.OkCustomParams(), @@ -140,7 +200,8 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { }, "missing strategy.customParams": { api.DeploymentConfig{ - Triggers: manualTrigger(), + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, + Triggers: manualTrigger(), Template: api.DeploymentTemplate{ Strategy: api.DeploymentStrategy{ Type: api.DeploymentStrategyTypeCustom, @@ -153,7 +214,8 @@ func TestValidateDeploymentConfigMissingFields(t *testing.T) { }, "missing template.strategy.customParams.image": { api.DeploymentConfig{ - Triggers: manualTrigger(), + ObjectMeta: kapi.ObjectMeta{Name: "foo", Namespace: "bar"}, + Triggers: manualTrigger(), Template: api.DeploymentTemplate{ Strategy: api.DeploymentStrategy{ Type: api.DeploymentStrategyTypeCustom, diff --git a/pkg/deploy/controller/config_change_controller.go b/pkg/deploy/controller/config_change_controller.go index 3be34f09ff86..32423031dae8 100644 --- a/pkg/deploy/controller/config_change_controller.go +++ b/pkg/deploy/controller/config_change_controller.go @@ -57,7 +57,7 @@ func (dc *DeploymentConfigChangeController) HandleDeploymentConfig() { return } - latestDeploymentID := deployutil.LatestDeploymentIDForConfig(config) + latestDeploymentID := deployutil.LatestDeploymentNameForConfig(config) obj, exists := dc.DeploymentStore.Get(latestDeploymentID) if !exists { @@ -67,14 +67,17 @@ func (dc *DeploymentConfigChangeController) HandleDeploymentConfig() { deployment := obj.(*kapi.ReplicationController) - if deployedConfig, err := deployutil.DecodeDeploymentConfig(deployment, dc.Codec); err == nil { - if deployutil.PodSpecsEqual(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec) { - glog.V(4).Infof("Ignoring updated config %s with LatestVersion=%d because it matches deployed config %s", config.Name, config.LatestVersion, deployment.Name) - return - } - } else { + deployedConfig, err := deployutil.DecodeDeploymentConfig(deployment, dc.Codec) + if err != nil { glog.V(0).Infof("Error decoding deploymentConfig from deployment %s: %v", deployment.Name, err) + return + } + + if deployutil.PodSpecsEqual(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec) { + glog.V(4).Infof("Ignoring updated config %s with LatestVersion=%d because it matches deployed config %s", config.Name, config.LatestVersion, deployment.Name) + return } + glog.V(4).Infof("Diff:\n%s", util.ObjectDiff(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec)) dc.generateDeployment(config, deployment) } @@ -86,6 +89,10 @@ func (dc *DeploymentConfigChangeController) generateDeployment(config *deployapi return } + if newConfig.LatestVersion == config.LatestVersion { + newConfig.LatestVersion++ + } + if deployment != nil { glog.V(4).Infof("Updating config %s (LatestVersion: %d -> %d) to advance existing deployment %s", config.Name, config.LatestVersion, newConfig.LatestVersion, deployment.Name) } diff --git a/pkg/deploy/controller/deployment_config_controller.go b/pkg/deploy/controller/deployment_config_controller.go index 85197c6cc38d..a003ab11db81 100644 --- a/pkg/deploy/controller/deployment_config_controller.go +++ b/pkg/deploy/controller/deployment_config_controller.go @@ -70,7 +70,7 @@ func (c *DeploymentConfigController) shouldDeploy(config *deployapi.DeploymentCo return false, nil } - latestDeploymentID := deployutil.LatestDeploymentIDForConfig(config) + latestDeploymentID := deployutil.LatestDeploymentNameForConfig(config) deployment, err := c.DeploymentInterface.GetDeployment(config.Namespace, latestDeploymentID) if err != nil { diff --git a/pkg/deploy/generator/config_generator.go b/pkg/deploy/generator/config_generator.go index af1696f90d77..145d5c4cd7fb 100644 --- a/pkg/deploy/generator/config_generator.go +++ b/pkg/deploy/generator/config_generator.go @@ -3,16 +3,14 @@ package generator import ( "fmt" - "github.com/golang/glog" - kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors" deployapi "github.com/openshift/origin/pkg/deploy/api" - deployutil "github.com/openshift/origin/pkg/deploy/util" imageapi "github.com/openshift/origin/pkg/image/api" ) @@ -20,138 +18,223 @@ import ( // and produces a DeploymentConfig which represents a potential future DeploymentConfig. If the generated // state differs from the input state, the LatestVersion field of the output is incremented. type DeploymentConfigGenerator struct { - DeploymentInterface deploymentInterface - DeploymentConfigInterface deploymentConfigInterface - ImageRepositoryInterface imageRepositoryInterface - Codec runtime.Codec + Client GeneratorClient + Codec runtime.Codec } -type deploymentInterface interface { - GetDeployment(ctx kapi.Context, id string) (*kapi.ReplicationController, error) +type GeneratorClient interface { + GetDeploymentConfig(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) + GetImageRepository(ctx kapi.Context, name string) (*imageapi.ImageRepository, error) + // LEGACY: used, to scan all repositories for a DockerImageReference. Will be removed + // when we drop support for reference by DockerImageReference. + ListImageRepositories(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) } -type deploymentConfigInterface interface { - GetDeploymentConfig(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) +type Client struct { + DCFn func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) + IRFn func(ctx kapi.Context, name string) (*imageapi.ImageRepository, error) + LIRFn func(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) + LIRFn2 func(ctx kapi.Context, label labels.Selector) (*imageapi.ImageRepositoryList, error) } -type imageRepositoryInterface interface { - ListImageRepositories(ctx kapi.Context, labels labels.Selector) (*imageapi.ImageRepositoryList, error) +func (c Client) GetDeploymentConfig(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + return c.DCFn(ctx, name) +} +func (c Client) GetImageRepository(ctx kapi.Context, name string) (*imageapi.ImageRepository, error) { + return c.IRFn(ctx, name) +} +func (c Client) ListImageRepositories(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) { + if c.LIRFn2 != nil { + return c.LIRFn2(ctx, labels.Everything()) + } + return c.LIRFn(ctx) } // Generate returns a potential future DeploymentConfig based on the DeploymentConfig specified -// by deploymentConfigID. -func (g *DeploymentConfigGenerator) Generate(ctx kapi.Context, deploymentConfigID string) (*deployapi.DeploymentConfig, error) { - glog.V(4).Infof("Generating new deployment config from deploymentConfig %v", deploymentConfigID) - - deploymentConfig, err := g.DeploymentConfigInterface.GetDeploymentConfig(ctx, deploymentConfigID) +// by namespace and name +func (g *DeploymentConfigGenerator) Generate(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + dc, err := g.Client.GetDeploymentConfig(ctx, name) if err != nil { - glog.V(4).Infof("Error getting deploymentConfig for id %v", deploymentConfigID) return nil, err } - deploymentID := deployutil.LatestDeploymentIDForConfig(deploymentConfig) - - deployment, err := g.DeploymentInterface.GetDeployment(ctx, deploymentID) - if err != nil && !errors.IsNotFound(err) { - glog.V(2).Infof("Error getting deployment: %#v", err) - return nil, err + refs, legacy := findReferences(dc) + if errs := retrieveReferences(g.Client, ctx, refs, legacy); len(errs) > 0 { + return nil, kerrors.NewAggregate(errs) + } + indexed := referencesByIndex(refs, legacy) + changed, errs := replaceReferences(dc, indexed) + if len(errs) > 0 { + return nil, kerrors.NewAggregate(errs) + } + if changed || dc.LatestVersion == 0 { + dc.LatestVersion++ } - deploymentExists := !errors.IsNotFound(err) - configPodTemplateSpec := deploymentConfig.Template.ControllerTemplate.Template + return dc, nil +} - referencedRepoNames := referencedRepoNames(deploymentConfig) - referencedRepos := imageReposByDockerImageRepo(ctx, g.ImageRepositoryInterface, referencedRepoNames) +type refKey struct { + namespace string + name string +} - for _, repoName := range referencedRepoNames.List() { - params := deployutil.ParamsForImageChangeTrigger(deploymentConfig, repoName) - repo, ok := referencedRepos[params.RepositoryName] - if !ok { - return nil, fmt.Errorf("Config references unknown ImageRepository '%s'", params.RepositoryName) +type triggerEntry struct { + positions []int + repo *imageapi.ImageRepository +} + +type triggersByRef map[refKey]*triggerEntry +type triggersByName map[string]*triggerEntry + +// findReferences looks up triggers with references and maps them back to their position in the trigger array. +func findReferences(dc *deployapi.DeploymentConfig) (refs triggersByRef, legacy triggersByName) { + refs, legacy = make(triggersByRef), make(triggersByName) + + for i := range dc.Triggers { + trigger := &dc.Triggers[i] + if trigger.Type != deployapi.DeploymentTriggerOnImageChange { + continue } - // TODO: If the tag is missing, what's the correct reaction? - tag, tagExists := repo.Tags[params.Tag] - if !tagExists { - glog.V(4).Infof("No tag %s found for repository %s (potentially invalid DeploymentConfig status)", tag, repoName) + // use the object reference to find the image repository + if from := &trigger.ImageChangeParams.From; len(from.Name) != 0 { + k := refKey{ + namespace: from.Namespace, + name: from.Name, + } + if len(k.namespace) == 0 { + k.namespace = dc.Namespace + } + trigger, ok := refs[k] + if !ok { + trigger = &triggerEntry{} + refs[k] = trigger + } + trigger.positions = append(trigger.positions, i) continue } - newImage := repo.DockerImageRepository + ":" + tag - updateContainers(configPodTemplateSpec, util.NewStringSet(params.ContainerNames...), newImage) + // use the old way of looking up the name + // DEPRECATED: this path will be removed soon + if k := trigger.ImageChangeParams.RepositoryName; len(k) != 0 { + trigger, ok := legacy[k] + if !ok { + trigger = &triggerEntry{} + legacy[k] = trigger + } + trigger.positions = append(trigger.positions, i) + continue + } } + return +} + +// retrieveReferences loads the repositories referenced by a deployment config +func retrieveReferences(client GeneratorClient, ctx kapi.Context, refs triggersByRef, legacy triggersByName) []error { + errs := []error{} - if deploymentExists { - if deployedConfig, err := deployutil.DecodeDeploymentConfig(deployment, g.Codec); err == nil { - if !deployutil.PodSpecsEqual(configPodTemplateSpec.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec) { - deploymentConfig.LatestVersion++ - // reset the details of the deployment trigger for this deploymentConfig - deploymentConfig.Details = nil - glog.V(4).Infof("Incremented deploymentConfig %s to %d due to template inequality with deployed config", deploymentConfig.Name, deploymentConfig.LatestVersion) - } else { - glog.V(4).Infof("No diff detected between deploymentConfig %s and deployed config %s", deploymentConfig.Name, deployedConfig.Name) + // fetch repositories directly + for k, v := range refs { + repo, err := client.GetImageRepository(kapi.WithNamespace(ctx, k.namespace), k.name) + if err != nil { + errs = append(errs, err) + continue + } + v.repo = repo + } + + // look for legacy references that we've already loaded + // DEPRECATED: remove all code below this line when the reference logic is removed + missing := make(triggersByName) + for k, v := range legacy { + for _, ref := range refs { + if ref.repo.Status.DockerImageRepository == k { + v.repo = ref.repo + break } - } else { - glog.V(0).Infof("Failed to decode DeploymentConfig from deployment %s: %v", deployment.Name, err) } - } else { - if deploymentConfig.LatestVersion == 0 { - // If the latest version is zero, and the generation's being called, bump it. - deploymentConfig.LatestVersion = 1 - // reset the details of the deployment trigger for this deploymentConfig - deploymentConfig.Details = nil - glog.V(4).Infof("Set deploymentConfig %s to version %d for initial deployment", deploymentConfig.Name, deploymentConfig.LatestVersion) + if v.repo == nil { + missing[k] = v } } - return deploymentConfig, nil -} - -func updateContainers(template *kapi.PodTemplateSpec, containers util.StringSet, newImage string) { - for i, container := range template.Spec.Containers { - if !containers.Has(container.Name) { - continue + // if we haven't loaded the references, do the more expensive list all + if len(missing) != 0 { + repos, err := client.ListImageRepositories(ctx) + if err != nil { + errs = append(errs, err) + return errs } - // TODO: If we grow beyond this single mutation, diffing hashes of - // a clone of the original config vs the mutation would be more generic. - if newImage != container.Image { - template.Spec.Containers[i].Image = newImage + for k, ref := range missing { + for i := range repos.Items { + repo := &repos.Items[i] + if repo.DockerImageRepository == k { + ref.repo = repo + break + } + } + if ref.repo == nil { + errs = append(errs, errors.NewFieldNotFound("dockerImageRepository", k)) + } } } + return errs } -func imageReposByDockerImageRepo(ctx kapi.Context, imageRepoInterface imageRepositoryInterface, filter *util.StringSet) map[string]imageapi.ImageRepository { - repos := make(map[string]imageapi.ImageRepository) +type reposByIndex map[int]*imageapi.ImageRepository - imageRepos, err := imageRepoInterface.ListImageRepositories(ctx, labels.Everything()) - if err != nil { - glog.V(2).Infof("Error listing imageRepositories: %#v", err) - return repos +func referencesByIndex(refs triggersByRef, legacy triggersByName) reposByIndex { + repos := make(reposByIndex) + for _, v := range refs { + for _, i := range v.positions { + repos[i] = v.repo + } } - - for _, repo := range imageRepos.Items { - if filter.Has(repo.DockerImageRepository) { - repos[repo.DockerImageRepository] = repo + for _, v := range legacy { + for _, i := range v.positions { + repos[i] = v.repo } } - return repos } -// Returns the image repositories names a config has triggers registered for -func referencedRepoNames(config *deployapi.DeploymentConfig) *util.StringSet { - repoIDs := &util.StringSet{} - - if config == nil || config.Triggers == nil { - return repoIDs - } +func replaceReferences(dc *deployapi.DeploymentConfig, repos reposByIndex) (changed bool, errs []error) { + template := dc.Template.ControllerTemplate.Template + for i, repo := range repos { + params := dc.Triggers[i].ImageChangeParams - for _, trigger := range config.Triggers { - if trigger.Type == deployapi.DeploymentTriggerOnImageChange { - repoIDs.Insert(trigger.ImageChangeParams.RepositoryName) + // lookup image id + tag := params.Tag + if len(tag) == 0 { + // TODO: replace with "preferred tag" from repo + tag = "latest" + } + id, ok := repo.Tags[tag] + if !ok { + errs = append(errs, fmt.Errorf("image repository %s/%s does not have tag %q", repo.Namespace, repo.Name)) + continue + } + if len(repo.Status.DockerImageRepository) == 0 { + errs = append(errs, fmt.Errorf("image repository %s/%s does not have a Docker image repository reference set and can't be used in a deployment config trigger", repo.Namespace, repo.Name)) + continue + } + // TODO: this assumes that tag value is the image id + image := fmt.Sprintf("%s:%s", repo.Status.DockerImageRepository, id) + + // update containers + names := util.NewStringSet(params.ContainerNames...) + for i := range template.Spec.Containers { + container := &template.Spec.Containers[i] + if !names.Has(container.Name) { + continue + } + if container.Image != image { + container.Image = image + changed = true + } } } - - return repoIDs + return } diff --git a/pkg/deploy/generator/config_generator_test.go b/pkg/deploy/generator/config_generator_test.go index b378a177c308..41c3a0b519be 100644 --- a/pkg/deploy/generator/config_generator_test.go +++ b/pkg/deploy/generator/config_generator_test.go @@ -1,11 +1,14 @@ package generator import ( + "strings" "testing" + "speter.net/go/exp/math/dec/inf" + kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" kerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" - "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" api "github.com/openshift/origin/pkg/api/latest" deployapi "github.com/openshift/origin/pkg/deploy/api" @@ -17,8 +20,8 @@ import ( func TestGenerateFromMissingDeploymentConfig(t *testing.T) { generator := &DeploymentConfigGenerator{ Codec: api.Codec, - DeploymentConfigInterface: &testDeploymentConfigInterface{ - GetDeploymentConfigFunc: func(id string) (*deployapi.DeploymentConfig, error) { + Client: Client{ + DCFn: func(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { return nil, kerrors.NewNotFound("deploymentConfig", id) }, }, @@ -38,20 +41,42 @@ func TestGenerateFromMissingDeploymentConfig(t *testing.T) { func TestGenerateFromConfigWithoutTagChange(t *testing.T) { generator := &DeploymentConfigGenerator{ Codec: api.Codec, - DeploymentConfigInterface: &testDeploymentConfigInterface{ - GetDeploymentConfigFunc: func(id string) (*deployapi.DeploymentConfig, error) { + Client: Client{ + DCFn: func(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { return deploytest.OkDeploymentConfig(1), nil }, - }, - ImageRepositoryInterface: &testImageRepositoryInterface{ - ListImageRepositoriesFunc: func(labels labels.Selector) (*imageapi.ImageRepositoryList, error) { + LIRFn: func(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) { return okImageRepoList(), nil }, }, - DeploymentInterface: &testDeploymentInterface{ - GetDeploymentFunc: func(id string) (*kapi.ReplicationController, error) { - deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) - return deployment, nil + } + + config, err := generator.Generate(kapi.NewDefaultContext(), "deploy1") + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if config == nil { + t.Fatalf("Expected non-nil config") + } + + if config.LatestVersion != 1 { + t.Fatalf("Expected config LatestVersion=1, got %d", config.LatestVersion) + } +} + +func TestGenerateFromZeroConfigWithoutTagChange(t *testing.T) { + dc := basicDeploymentConfig() + dc.LatestVersion = 0 + generator := &DeploymentConfigGenerator{ + Codec: api.Codec, + Client: Client{ + DCFn: func(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { + return dc, nil + }, + LIRFn: func(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) { + return okImageRepoList(), nil }, }, } @@ -74,21 +99,14 @@ func TestGenerateFromConfigWithoutTagChange(t *testing.T) { func TestGenerateFromConfigWithNoDeployment(t *testing.T) { generator := &DeploymentConfigGenerator{ Codec: api.Codec, - DeploymentConfigInterface: &testDeploymentConfigInterface{ - GetDeploymentConfigFunc: func(id string) (*deployapi.DeploymentConfig, error) { + Client: Client{ + DCFn: func(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { return deploytest.OkDeploymentConfig(1), nil }, - }, - ImageRepositoryInterface: &testImageRepositoryInterface{ - ListImageRepositoriesFunc: func(labels labels.Selector) (*imageapi.ImageRepositoryList, error) { + LIRFn: func(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) { return okImageRepoList(), nil }, }, - DeploymentInterface: &testDeploymentInterface{ - GetDeploymentFunc: func(id string) (*kapi.ReplicationController, error) { - return nil, kerrors.NewNotFound("replicationController", id) - }, - }, } config, err := generator.Generate(kapi.NewDefaultContext(), "deploy2") @@ -109,22 +127,69 @@ func TestGenerateFromConfigWithNoDeployment(t *testing.T) { func TestGenerateFromConfigWithUpdatedImageRef(t *testing.T) { generator := &DeploymentConfigGenerator{ Codec: api.Codec, - DeploymentConfigInterface: &testDeploymentConfigInterface{ - GetDeploymentConfigFunc: func(id string) (*deployapi.DeploymentConfig, error) { + Client: Client{ + DCFn: func(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { return deploytest.OkDeploymentConfig(1), nil }, - }, - ImageRepositoryInterface: &testImageRepositoryInterface{ - ListImageRepositoriesFunc: func(labels labels.Selector) (*imageapi.ImageRepositoryList, error) { + LIRFn: func(ctx kapi.Context) (*imageapi.ImageRepositoryList, error) { list := okImageRepoList() list.Items[0].Tags["tag1"] = "ref2" return list, nil }, }, - DeploymentInterface: &testDeploymentInterface{ - GetDeploymentFunc: func(id string) (*kapi.ReplicationController, error) { - deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) - return deployment, nil + } + + config, err := generator.Generate(kapi.NewDefaultContext(), "deploy1") + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if config == nil { + t.Fatalf("Expected non-nil config") + } + + if config.LatestVersion != 2 { + t.Fatalf("Expected config LatestVersion=2, got %d", config.LatestVersion) + } + + expected := "registry:8080/repo1:ref2" + actual := config.Template.ControllerTemplate.Template.Spec.Containers[0].Image + if expected != actual { + t.Fatalf("Expected container image %s, got %s", expected, actual) + } +} + +func TestGenerateReportsErrorWhenRepoHasNoImage(t *testing.T) { + generator := &DeploymentConfigGenerator{ + Codec: api.Codec, + Client: Client{ + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + return referenceDeploymentConfig(), nil + }, + IRFn: func(ctx kapi.Context, name string) (*imageapi.ImageRepository, error) { + return &emptyImageRepo().Items[0], nil + }, + }, + } + _, err := generator.Generate(kapi.NewDefaultContext(), "deploy1") + if err == nil { + t.Fatalf("Unexpected non-error") + } + if !strings.Contains(err.Error(), "image repository /imageRepo1 does not have a Docker") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestGenerateDeploymentConfigWithFrom(t *testing.T) { + generator := &DeploymentConfigGenerator{ + Codec: api.Codec, + Client: Client{ + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + return referenceDeploymentConfig(), nil + }, + IRFn: func(ctx kapi.Context, name string) (*imageapi.ImageRepository, error) { + return &internalImageRepo().Items[0], nil }, }, } @@ -143,46 +208,148 @@ func TestGenerateFromConfigWithUpdatedImageRef(t *testing.T) { t.Fatalf("Expected config LatestVersion=2, got %d", config.LatestVersion) } - expected := "registry:8080/repo1:ref2" + expected := "internal/namespace/imageRepo1:ref1" actual := config.Template.ControllerTemplate.Template.Spec.Containers[0].Image if expected != actual { t.Fatalf("Expected container image %s, got %s", expected, actual) } } -type testDeploymentInterface struct { - GetDeploymentFunc func(id string) (*kapi.ReplicationController, error) +func okImageRepoList() *imageapi.ImageRepositoryList { + return &imageapi.ImageRepositoryList{ + Items: []imageapi.ImageRepository{ + { + ObjectMeta: kapi.ObjectMeta{Name: "imageRepo1"}, + DockerImageRepository: "registry:8080/repo1", + Tags: map[string]string{ + "tag1": "ref1", + }, + Status: imageapi.ImageRepositoryStatus{ + DockerImageRepository: "registry:8080/repo1", + }, + }, + }, + } } -func (i *testDeploymentInterface) GetDeployment(ctx kapi.Context, id string) (*kapi.ReplicationController, error) { - return i.GetDeploymentFunc(id) +func basicPodTemplate() *kapi.PodTemplateSpec { + return &kapi.PodTemplateSpec{ + Spec: kapi.PodSpec{ + Containers: []kapi.Container{ + { + Name: "container1", + Image: "registry:8080/repo1:ref1", + CPU: resource.Quantity{Amount: inf.NewDec(0, 3), Format: "DecimalSI"}, + Memory: resource.Quantity{Amount: inf.NewDec(0, 0), Format: "DecimalSI"}, + }, + { + Name: "container2", + Image: "registry:8080/repo1:ref2", + CPU: resource.Quantity{Amount: inf.NewDec(0, 3), Format: "DecimalSI"}, + Memory: resource.Quantity{Amount: inf.NewDec(0, 0), Format: "DecimalSI"}, + }, + }, + }, + } } -type testDeploymentConfigInterface struct { - GetDeploymentConfigFunc func(id string) (*deployapi.DeploymentConfig, error) +func basicDeploymentConfig() *deployapi.DeploymentConfig { + return &deployapi.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "deploy1"}, + LatestVersion: 1, + Triggers: []deployapi.DeploymentTriggerPolicy{ + { + Type: deployapi.DeploymentTriggerOnImageChange, + ImageChangeParams: &deployapi.DeploymentTriggerImageChangeParams{ + ContainerNames: []string{ + "container1", + }, + RepositoryName: "registry:8080/repo1", + Tag: "tag1", + }, + }, + }, + Template: deployapi.DeploymentTemplate{ + ControllerTemplate: kapi.ReplicationControllerSpec{ + Template: basicPodTemplate(), + }, + }, + } } -func (i *testDeploymentConfigInterface) GetDeploymentConfig(ctx kapi.Context, id string) (*deployapi.DeploymentConfig, error) { - return i.GetDeploymentConfigFunc(id) +func referenceDeploymentConfig() *deployapi.DeploymentConfig { + return &deployapi.DeploymentConfig{ + ObjectMeta: kapi.ObjectMeta{Name: "deploy1"}, + LatestVersion: 1, + Triggers: []deployapi.DeploymentTriggerPolicy{ + { + Type: deployapi.DeploymentTriggerOnImageChange, + ImageChangeParams: &deployapi.DeploymentTriggerImageChangeParams{ + ContainerNames: []string{ + "container1", + }, + From: kapi.ObjectReference{ + Name: "repo1", + }, + Tag: "tag1", + }, + }, + }, + Template: deployapi.DeploymentTemplate{ + ControllerTemplate: kapi.ReplicationControllerSpec{ + Template: basicPodTemplate(), + }, + }, + } } -type testImageRepositoryInterface struct { - ListImageRepositoriesFunc func(labels labels.Selector) (*imageapi.ImageRepositoryList, error) +func basicDeployment() *kapi.ReplicationController { + config := basicDeploymentConfig() + encodedConfig, _ := deployutil.EncodeDeploymentConfig(config, api.Codec) + + return &kapi.ReplicationController{ + ObjectMeta: kapi.ObjectMeta{ + Name: deployutil.LatestDeploymentNameForConfig(config), + Annotations: map[string]string{ + deployapi.DeploymentConfigAnnotation: config.Name, + deployapi.DeploymentStatusAnnotation: string(deployapi.DeploymentStatusNew), + deployapi.DeploymentEncodedConfigAnnotation: encodedConfig, + }, + Labels: config.Labels, + }, + Spec: kapi.ReplicationControllerSpec{ + Template: basicPodTemplate(), + }, + } } -func (i *testImageRepositoryInterface) ListImageRepositories(ctx kapi.Context, labels labels.Selector) (*imageapi.ImageRepositoryList, error) { - return i.ListImageRepositoriesFunc(labels) +func internalImageRepo() *imageapi.ImageRepositoryList { + return &imageapi.ImageRepositoryList{ + Items: []imageapi.ImageRepository{ + { + ObjectMeta: kapi.ObjectMeta{Name: "imageRepo1"}, + Tags: map[string]string{ + "tag1": "ref1", + }, + Status: imageapi.ImageRepositoryStatus{ + DockerImageRepository: "internal/namespace/imageRepo1", + }, + }, + }, + } } -func okImageRepoList() *imageapi.ImageRepositoryList { +func emptyImageRepo() *imageapi.ImageRepositoryList { return &imageapi.ImageRepositoryList{ Items: []imageapi.ImageRepository{ { - ObjectMeta: kapi.ObjectMeta{Name: "imageRepo1"}, - DockerImageRepository: "registry:8080/repo1", + ObjectMeta: kapi.ObjectMeta{Name: "imageRepo1"}, Tags: map[string]string{ "tag1": "ref1", }, + Status: imageapi.ImageRepositoryStatus{ + DockerImageRepository: "", + }, }, }, } diff --git a/pkg/deploy/rollback/rest.go b/pkg/deploy/rollback/rest.go index dfcd83d9f5f3..eac02d0a24de 100644 --- a/pkg/deploy/rollback/rest.go +++ b/pkg/deploy/rollback/rest.go @@ -15,34 +15,39 @@ import ( // REST provides a rollback generation endpoint. Only the Create method is implemented. type REST struct { - generator GeneratorClient - deploymentGetter DeploymentGetter - deploymentConfigGetter DeploymentConfigGetter - codec runtime.Codec + generator GeneratorClient + codec runtime.Codec } // GeneratorClient defines a local interface to a rollback generator for testability. type GeneratorClient interface { - Generate(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) + GenerateRollback(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) + GetDeployment(ctx kapi.Context, name string) (*kapi.ReplicationController, error) + GetDeploymentConfig(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) } -// DeploymentGetter is a local interface to ReplicationControllers for testability. -type DeploymentGetter interface { - GetDeployment(namespace, name string) (*kapi.ReplicationController, error) +// Client provides an implementation of Generator client +type Client struct { + GRFn func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) + RCFn func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) + DCFn func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) } -// DeploymentConfigGetter is a local interface to DeploymentConfigs for testability. -type DeploymentConfigGetter interface { - GetDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error) +func (c Client) GetDeploymentConfig(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + return c.DCFn(ctx, name) +} +func (c Client) GetDeployment(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { + return c.RCFn(ctx, name) +} +func (c Client) GenerateRollback(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + return c.GRFn(from, to, spec) } // NewREST safely creates a new REST. -func NewREST(generator GeneratorClient, deploymentGetter DeploymentGetter, configGetter DeploymentConfigGetter, codec runtime.Codec) apiserver.RESTStorage { +func NewREST(generator GeneratorClient, codec runtime.Codec) apiserver.RESTStorage { return &REST{ - generator: generator, - deploymentGetter: deploymentGetter, - deploymentConfigGetter: configGetter, - codec: codec, + generator: generator, + codec: codec, } } @@ -61,33 +66,28 @@ func (s *REST) Create(ctx kapi.Context, obj runtime.Object) (<-chan apiserver.RE return nil, kerrors.NewInvalid("deploymentConfigRollback", "", errs) } - namespace, namespaceOk := kapi.NamespaceFrom(ctx) - if !namespaceOk { - return nil, fmt.Errorf("namespace %s is invalid", ctx.Value) - } - // Roll back "from" the current deployment "to" a target deployment - var from, to *deployapi.DeploymentConfig - var err error // Find the target ("to") deployment and decode the DeploymentConfig - if targetDeployment, err := s.deploymentGetter.GetDeployment(namespace, rollback.Spec.From.Name); err != nil { + targetDeployment, err := s.generator.GetDeployment(ctx, rollback.Spec.From.Name) + if err != nil { // TODO: correct error type? return nil, kerrors.NewBadRequest(fmt.Sprintf("Couldn't get specified deployment: %v", err)) - } else { - if to, err = deployutil.DecodeDeploymentConfig(targetDeployment, s.codec); err != nil { - // TODO: correct error type? - return nil, kerrors.NewBadRequest(fmt.Sprintf("deploymentConfig on target deployment is invalid: %v", err)) - } + } + to, err := deployutil.DecodeDeploymentConfig(targetDeployment, s.codec) + if err != nil { + // TODO: correct error type? + return nil, kerrors.NewBadRequest(fmt.Sprintf("deploymentConfig on target deployment is invalid: %v", err)) } // Find the current ("from") version of the target deploymentConfig - if from, err = s.deploymentConfigGetter.GetDeploymentConfig(namespace, to.Name); err != nil { + from, err := s.generator.GetDeploymentConfig(ctx, to.Name) + if err != nil { // TODO: correct error type? - return nil, kerrors.NewBadRequest(fmt.Sprintf("Couldn't find current deploymentConfig %s/%s: %v", namespace, to.Name, err)) + return nil, kerrors.NewBadRequest(fmt.Sprintf("Couldn't find current deploymentConfig %s/%s: %v", targetDeployment.Namespace, to.Name, err)) } return apiserver.MakeAsync(func() (runtime.Object, error) { - return s.generator.Generate(from, to, &rollback.Spec) + return s.generator.GenerateRollback(from, to, &rollback.Spec) }), nil } diff --git a/pkg/deploy/rollback/rest_test.go b/pkg/deploy/rollback/rest_test.go index 6016496b9cb2..df95536a2785 100644 --- a/pkg/deploy/rollback/rest_test.go +++ b/pkg/deploy/rollback/rest_test.go @@ -16,7 +16,6 @@ import ( func TestCreateError(t *testing.T) { rest := REST{} - obj, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfig{}) if err == nil { @@ -30,7 +29,6 @@ func TestCreateError(t *testing.T) { func TestCreateInvalid(t *testing.T) { rest := REST{} - obj, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{}) if err == nil { @@ -44,23 +42,19 @@ func TestCreateInvalid(t *testing.T) { func TestCreateOk(t *testing.T) { rest := REST{ - generator: &testGenerator{ - generateFunc: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + generator: Client{ + GRFn: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { return &deployapi.DeploymentConfig{}, nil }, - }, - codec: api.Codec, - deploymentGetter: &testDeploymentGetter{ - GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) { + RCFn: func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) return deployment, nil }, - }, - deploymentConfigGetter: &testDeploymentConfigGetter{ - GetDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) { + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { return deploytest.OkDeploymentConfig(1), nil }, }, + codec: api.Codec, } channel, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{ @@ -92,23 +86,19 @@ func TestCreateOk(t *testing.T) { func TestCreateGeneratorError(t *testing.T) { rest := REST{ - generator: &testGenerator{ - generateFunc: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + generator: Client{ + GRFn: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { return nil, errors.New("something terrible happened") }, - }, - codec: api.Codec, - deploymentGetter: &testDeploymentGetter{ - GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) { + RCFn: func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) return deployment, nil }, - }, - deploymentConfigGetter: &testDeploymentConfigGetter{ - GetDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) { + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { return deploytest.OkDeploymentConfig(1), nil }, }, + codec: api.Codec, } channel, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{ @@ -144,24 +134,21 @@ func TestCreateGeneratorError(t *testing.T) { func TestCreateMissingDeployment(t *testing.T) { rest := REST{ - generator: &testGenerator{ - generateFunc: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + generator: Client{ + GRFn: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { t.Fatal("unexpected call to generator") return nil, errors.New("something terrible happened") }, - }, - codec: api.Codec, - deploymentGetter: &testDeploymentGetter{ - GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) { + RCFn: func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { return nil, kerrors.NewNotFound("replicationController", name) }, - }, - deploymentConfigGetter: &testDeploymentConfigGetter{ - GetDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) { + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + namespace, _ := kapi.NamespaceFrom(ctx) t.Fatalf("unexpected call to GetDeploymentConfig(%s/%s)", namespace, name) return nil, kerrors.NewNotFound("deploymentConfig", name) }, }, + codec: api.Codec, } channel, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{ @@ -184,27 +171,24 @@ func TestCreateMissingDeployment(t *testing.T) { func TestCreateInvalidDeployment(t *testing.T) { rest := REST{ - generator: &testGenerator{ - generateFunc: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + generator: Client{ + GRFn: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { t.Fatal("unexpected call to generator") return nil, errors.New("something terrible happened") }, - }, - codec: api.Codec, - deploymentGetter: &testDeploymentGetter{ - GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) { + RCFn: func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { // invalidate the encoded config deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) deployment.Annotations[deployapi.DeploymentEncodedConfigAnnotation] = "" return deployment, nil }, - }, - deploymentConfigGetter: &testDeploymentConfigGetter{ - GetDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) { + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { + namespace, _ := kapi.NamespaceFrom(ctx) t.Fatalf("unexpected call to GetDeploymentConfig(%s/%s)", namespace, name) return nil, kerrors.NewNotFound("deploymentConfig", name) }, }, + codec: api.Codec, } channel, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{ @@ -227,24 +211,20 @@ func TestCreateInvalidDeployment(t *testing.T) { func TestCreateMissingDeploymentConfig(t *testing.T) { rest := REST{ - generator: &testGenerator{ - generateFunc: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { + generator: Client{ + GRFn: func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { t.Fatal("unexpected call to generator") return nil, errors.New("something terrible happened") }, - }, - codec: api.Codec, - deploymentGetter: &testDeploymentGetter{ - GetDeploymentFunc: func(namespace, name string) (*kapi.ReplicationController, error) { + RCFn: func(ctx kapi.Context, name string) (*kapi.ReplicationController, error) { deployment, _ := deployutil.MakeDeployment(deploytest.OkDeploymentConfig(1), kapi.Codec) return deployment, nil }, - }, - deploymentConfigGetter: &testDeploymentConfigGetter{ - GetDeploymentConfigFunc: func(namespace, name string) (*deployapi.DeploymentConfig, error) { + DCFn: func(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) { return nil, kerrors.NewNotFound("deploymentConfig", name) }, }, + codec: api.Codec, } channel, err := rest.Create(kapi.NewDefaultContext(), &deployapi.DeploymentConfigRollback{ @@ -267,30 +247,6 @@ func TestCreateMissingDeploymentConfig(t *testing.T) { func TestNew(t *testing.T) { // :) - rest := NewREST(&testGenerator{}, &testDeploymentGetter{}, &testDeploymentConfigGetter{}, api.Codec) + rest := NewREST(Client{}, api.Codec) rest.New() } - -type testGenerator struct { - generateFunc func(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) -} - -func (g *testGenerator) Generate(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { - return g.generateFunc(from, to, spec) -} - -type testDeploymentGetter struct { - GetDeploymentFunc func(namespace, name string) (*kapi.ReplicationController, error) -} - -func (i *testDeploymentGetter) GetDeployment(namespace, name string) (*kapi.ReplicationController, error) { - return i.GetDeploymentFunc(namespace, name) -} - -type testDeploymentConfigGetter struct { - GetDeploymentConfigFunc func(namespace, name string) (*deployapi.DeploymentConfig, error) -} - -func (i *testDeploymentConfigGetter) GetDeploymentConfig(namespace, name string) (*deployapi.DeploymentConfig, error) { - return i.GetDeploymentConfigFunc(namespace, name) -} diff --git a/pkg/deploy/rollback/rollback_generator.go b/pkg/deploy/rollback/rollback_generator.go index b0bf41714e09..6a642639e8dd 100644 --- a/pkg/deploy/rollback/rollback_generator.go +++ b/pkg/deploy/rollback/rollback_generator.go @@ -10,13 +10,12 @@ import ( // RollbackGenerator generates a new DeploymentConfig by merging a pair of DeploymentConfigs // in a configurable way. -type RollbackGenerator struct { -} +type RollbackGenerator struct{} // Generate creates a new DeploymentConfig by merging to onto from based on the options provided // by spec. The LatestVersion of the result is unconditionally incremented, as rollback candidates are // should be possible to be deployed manually regardless of other system behavior such as triggering. -func (g *RollbackGenerator) Generate(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { +func (g *RollbackGenerator) GenerateRollback(from, to *deployapi.DeploymentConfig, spec *deployapi.DeploymentConfigRollbackSpec) (*deployapi.DeploymentConfig, error) { rollback := &deployapi.DeploymentConfig{} if err := kapi.Scheme.Convert(&from, &rollback); err != nil { diff --git a/pkg/deploy/rollback/rollback_generator_test.go b/pkg/deploy/rollback/rollback_generator_test.go index 10a84f9035fc..6fadc278c29b 100644 --- a/pkg/deploy/rollback/rollback_generator_test.go +++ b/pkg/deploy/rollback/rollback_generator_test.go @@ -47,7 +47,7 @@ func TestGeneration(t *testing.T) { for _, spec := range rollbackSpecs { t.Logf("testing spec %#v", spec) - if rollback, err := generator.Generate(from, to, spec); err != nil { + if rollback, err := generator.GenerateRollback(from, to, spec); err != nil { t.Fatalf("Unexpected error: %v", err) } else { if hasStrategyDiff(from, rollback) && !spec.IncludeStrategy { diff --git a/pkg/deploy/util/util.go b/pkg/deploy/util/util.go index 3cab436d7cdd..ad3f7a3b7de4 100644 --- a/pkg/deploy/util/util.go +++ b/pkg/deploy/util/util.go @@ -5,80 +5,25 @@ import ( "fmt" "hash/adler32" "strconv" - "strings" "github.com/golang/glog" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - "github.com/GoogleCloudPlatform/kubernetes/pkg/util" deployapi "github.com/openshift/origin/pkg/deploy/api" ) -// LatestDeploymentIDForConfig returns a stable identifier for config based on its version. -func LatestDeploymentIDForConfig(config *deployapi.DeploymentConfig) string { +// LatestDeploymentNameForConfig returns a stable identifier for config based on its version. +func LatestDeploymentNameForConfig(config *deployapi.DeploymentConfig) string { return config.Name + "-" + strconv.Itoa(config.LatestVersion) } -func ParamsForImageChangeTrigger(config *deployapi.DeploymentConfig, repoName string) *deployapi.DeploymentTriggerImageChangeParams { - if config == nil || config.Triggers == nil { - return nil - } - - for _, trigger := range config.Triggers { - if trigger.Type == deployapi.DeploymentTriggerOnImageChange && trigger.ImageChangeParams.RepositoryName == repoName { - return trigger.ImageChangeParams - } - } - - return nil -} - -// Set a-b -func Difference(a, b util.StringSet) util.StringSet { - diff := util.StringSet{} - - if a == nil || b == nil { - return diff - } - - for _, s := range a.List() { - if !b.Has(s) { - diff.Insert(s) - } - } - - return diff -} - -// Returns a map of referenced image name to image version -func ReferencedImages(deployment *deployapi.Deployment) map[string]string { - result := make(map[string]string) - - if deployment == nil { - return result - } - - for _, container := range deployment.ControllerTemplate.Template.Spec.Containers { - name, version := ParseContainerImage(container.Image) - result[name] = version - } - - return result -} - -func ParseContainerImage(image string) (string, string) { - tokens := strings.Split(image, ":") - return tokens[0], tokens[1] -} - // HashPodSpecs hashes a PodSpec into a uint64. // TODO: Resources are currently ignored due to the formats not surviving encoding/decoding // in a consistent manner (e.g. 0 is represented sometimes as 0.000) func HashPodSpec(t api.PodSpec) uint64 { - for i := range t.Containers { t.Containers[i].CPU = resource.Quantity{} t.Containers[i].Memory = resource.Quantity{} @@ -135,7 +80,7 @@ func MakeDeployment(config *deployapi.DeploymentConfig, codec runtime.Codec) (*a deployment := &api.ReplicationController{ ObjectMeta: api.ObjectMeta{ - Name: LatestDeploymentIDForConfig(config), + Name: LatestDeploymentNameForConfig(config), Annotations: map[string]string{ deployapi.DeploymentConfigAnnotation: config.Name, deployapi.DeploymentStatusAnnotation: string(deployapi.DeploymentStatusNew), diff --git a/test/integration/buildclient_test.go b/test/integration/buildclient_test.go index 9485e09aff61..c70e72f8818b 100644 --- a/test/integration/buildclient_test.go +++ b/test/integration/buildclient_test.go @@ -151,6 +151,8 @@ func NewTestBuildOpenshift(t *testing.T) *testBuildOpenshift { stop: make(chan struct{}), } + openshift.lock.Lock() + defer openshift.lock.Unlock() etcdClient := newEtcdClient() etcdHelper, _ := master.NewEtcdHelper(etcdClient, klatest.Version) diff --git a/test/integration/deploy_trigger_test.go b/test/integration/deploy_trigger_test.go index a04ebb31ca1c..9ce975d95cbc 100644 --- a/test/integration/deploy_trigger_test.go +++ b/test/integration/deploy_trigger_test.go @@ -7,6 +7,8 @@ import ( "net/http/httptest" "testing" + "github.com/golang/glog" + kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" klatest "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" "github.com/GoogleCloudPlatform/kubernetes/pkg/apiserver" @@ -68,13 +70,13 @@ func TestSuccessfulManualDeployment(t *testing.T) { if config.LatestVersion != 1 { t.Fatalf("Generated deployment should have version 1: %#v", config) } - t.Logf("config(1): %#v", config) + glog.Infof("config(1): %#v", config) new, err := openshift.Client.DeploymentConfigs(testNamespace).Update(config) if err != nil { t.Fatalf("Couldn't create updated DeploymentConfig: %v %#v", err, config) } - t.Logf("config(2): %#v", new) + glog.Infof("config(2): %#v", new) event := <-watch.ResultChan() if e, a := watchapi.Added, event.Type; e != a { @@ -112,8 +114,75 @@ func TestSimpleImageChangeTrigger(t *testing.T) { } defer watch.Stop() - imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo) + if imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo); err != nil { + t.Fatalf("Couldn't create ImageRepository: %v", err) + } + + if _, err := openshift.Client.DeploymentConfigs(testNamespace).Create(config); err != nil { + t.Fatalf("Couldn't create DeploymentConfig: %v", err) + } + + if config, err = openshift.Client.DeploymentConfigs(testNamespace).Generate(config.Name); err != nil { + t.Fatalf("Error generating config: %v", err) + } + + if _, err := openshift.Client.DeploymentConfigs(testNamespace).Update(config); err != nil { + t.Fatalf("Couldn't create updated DeploymentConfig: %v", err) + } + + event := <-watch.ResultChan() + if e, a := watchapi.Added, event.Type; e != a { + t.Fatalf("expected watch event type %s, got %s", e, a) + } + deployment := event.Object.(*kapi.ReplicationController) + + if e, a := config.Name, deployment.Annotations[deployapi.DeploymentConfigAnnotation]; e != a { + t.Fatalf("Expected deployment annotated with deploymentConfig '%s', got '%s'", e, a) + } + + imageRepo.Tags["latest"] = "ref-2" + + if _, err = openshift.Client.ImageRepositories(testNamespace).Update(imageRepo); err != nil { + t.Fatalf("Error updating imageRepo: %v", err) + } + + event = <-watch.ResultChan() + if e, a := watchapi.Added, event.Type; e != a { + t.Fatalf("expected watch event type %s, got %s", e, a) + } + newDeployment := event.Object.(*kapi.ReplicationController) + + if newDeployment.Name == deployment.Name { + t.Fatalf("expected new deployment; old=%s, new=%s", deployment.Name, newDeployment.Name) + } +} + +func TestSimpleImageChangeTriggerFrom(t *testing.T) { + deleteAllEtcdKeys() + openshift := NewTestOpenshift(t) + defer openshift.Close() + + imageRepo := &imageapi.ImageRepository{ + ObjectMeta: kapi.ObjectMeta{Name: "test-image-repo"}, + Tags: map[string]string{ + "latest": "ref-1", + }, + } + + config := imageChangeDeploymentConfig() + config.Triggers[0].ImageChangeParams.RepositoryName = "" + config.Triggers[0].ImageChangeParams.From = kapi.ObjectReference{ + Name: "test-image-repo", + } + var err error + + watch, err := openshift.KubeClient.ReplicationControllers(testNamespace).Watch(labels.Everything(), labels.Everything(), "0") if err != nil { + t.Fatalf("Couldn't subscribe to Deployments %v", err) + } + defer watch.Stop() + + if imageRepo, err = openshift.Client.ImageRepositories(testNamespace).Create(imageRepo); err != nil { t.Fatalf("Couldn't create ImageRepository: %v", err) } @@ -154,6 +223,9 @@ func TestSimpleImageChangeTrigger(t *testing.T) { if newDeployment.Name == deployment.Name { t.Fatalf("expected new deployment; old=%s, new=%s", deployment.Name, newDeployment.Name) } + if a, e := newDeployment.Spec.Template.Spec.Containers[0].Image, "registry:3000/integration-test/test-image-repo:ref-2"; e != a { + t.Fatalf("new deployment isn't pointing to the right image: %s %s", e, a) + } } func TestSimpleConfigChangeTrigger(t *testing.T) { @@ -279,10 +351,12 @@ func NewTestOpenshift(t *testing.T) *testOpenshift { imageEtcd := imageetcd.New(etcdHelper, imageetcd.DefaultRegistryFunc(func() (string, bool) { return "registry:3000", true })) deployEtcd := deployetcd.New(etcdHelper) deployConfigGenerator := &deployconfiggenerator.DeploymentConfigGenerator{ - Codec: latest.Codec, - DeploymentInterface: &clientDeploymentInterface{kubeClient}, - DeploymentConfigInterface: deployEtcd, - ImageRepositoryInterface: imageEtcd, + Client: deployconfiggenerator.Client{ + DCFn: deployEtcd.GetDeploymentConfig, + IRFn: imageEtcd.GetImageRepository, + LIRFn2: imageEtcd.ListImageRepositories, + }, + Codec: latest.Codec, } buildEtcd := buildetcd.New(etcdHelper) From 366baa89760d808cb195b8957ca8e3718595a719 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 21 Jan 2015 22:27:07 -0500 Subject: [PATCH 14/18] More flexibility to test-cmd and test-end-to-end Use quotes everywhere, fix some Linux/Mac wierdnesses. Finally put all the e2e output under a single dir. Add `make release` --- Makefile | 8 +++++++ hack/test-cmd.sh | 30 +++++++++++------------- hack/test-end-to-end.sh | 52 +++++++++++++++++++++-------------------- 3 files changed, 49 insertions(+), 41 deletions(-) diff --git a/Makefile b/Makefile index 116ccff298a7..e58d5d2d08a0 100644 --- a/Makefile +++ b/Makefile @@ -77,3 +77,11 @@ clean: rm -rf $(OUT_DIR) $(OUT_PKG_DIR) .PHONY: clean +# Build an official release of OpenShift, including the official images. +# +# Example: +# make clean +release: clean + hack/build-release.sh + hack/build-images.sh +.PHONY: release diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 114950923395..943eb3d0833f 100755 --- a/hack/test-cmd.sh +++ b/hack/test-cmd.sh @@ -37,7 +37,7 @@ KUBELET_PORT=${KUBELET_PORT:-10250} ETCD_DATA_DIR=$(mktemp -d /tmp/openshift.local.etcd.XXXX) VOLUME_DIR=$(mktemp -d /tmp/openshift.local.volumes.XXXX) -CERT_DIR=$(mktemp -d /tmp/openshift.local.certificates.XXXX) +CERT_DIR="${CERT_DIR:-$(mktemp -d /tmp/openshift.local.certificates.XXXX)}" # set path so OpenShift is available GO_OUT="${OS_ROOT}/_output/local/go/bin" @@ -48,26 +48,22 @@ out=$(openshift version) echo openshift: $out # Start openshift -if [[ "$API_SCHEME" == "https" ]]; then - export CURL_CA_BUNDLE="$CERT_DIR/admin/root.crt" -fi - -openshift start --master="${API_SCHEME}://${API_HOST}:${API_PORT}" --listen="${API_SCHEME}://0.0.0.0:${API_PORT}" --hostname=${API_HOST} --volume-dir="${VOLUME_DIR}" --cert-dir="${CERT_DIR}" --etcd-dir="${ETCD_DATA_DIR}" 1>&2 & +openshift start --master="${API_SCHEME}://${API_HOST}:${API_PORT}" --listen="${API_SCHEME}://${API_HOST}:${API_PORT}" --hostname="${API_HOST}" --volume-dir="${VOLUME_DIR}" --cert-dir="${CERT_DIR}" --etcd-dir="${ETCD_DATA_DIR}" 1>&2 & OS_PID=$! -wait_for_url "http://localhost:${KUBELET_PORT}/healthz" "kubelet: " 0.2 60 -wait_for_url "http://${API_HOST}:${API_PORT}/healthz" "apiserver: " 0.2 60 -wait_for_url "http://${API_HOST}:${API_PORT}/api/v1beta1/minions/127.0.0.1" "apiserver(minions): " 0.2 60 - -wait_for_url "${KUBELET_SCHEME}://${API_HOST}:${KUBELET_PORT}/healthz" "kubelet: " 1 30 -wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz" "apiserver: " +if [[ "${API_SCHEME}" == "https" ]]; then + export CURL_CA_BUNDLE="${CERT_DIR}/admin/root.crt" +fi +wait_for_url "http://${API_HOST}:${KUBELET_PORT}/healthz" "kubelet: " 0.2 60 +wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz" "apiserver: " 0.2 60 +wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/api/v1beta1/minions/127.0.0.1" "apiserver(minions): " 0.2 60 # Set KUBERNETES_MASTER for osc export KUBERNETES_MASTER="${API_SCHEME}://${API_HOST}:${API_PORT}" -if [[ "$API_SCHEME" == "https" ]]; then - # Make osc use $CERT_DIR/admin/.kubeconfig, and ignore anything in the running user's $HOME dir - export HOME=$CERT_DIR/admin - export KUBECONFIG=$CERT_DIR/admin/.kubeconfig +if [[ "${API_SCHEME}" == "https" ]]; then + # Make osc use ${CERT_DIR}/admin/.kubeconfig, and ignore anything in the running user's $HOME dir + export HOME="${CERT_DIR}/admin" + export KUBECONFIG="${CERT_DIR}/admin/.kubeconfig" fi # @@ -129,3 +125,5 @@ echo "start-build: ok" osc cancel-build "${started}" --dump-logs --restart echo "cancel-build: ok" + +osc get minions,pods diff --git a/hack/test-end-to-end.sh b/hack/test-end-to-end.sh index cca78423f34a..28619f41691b 100755 --- a/hack/test-end-to-end.sh +++ b/hack/test-end-to-end.sh @@ -29,11 +29,12 @@ USE_LOCAL_IMAGES="${USE_LOCAL_IMAGES:-true}" ROUTER_TESTS_ENABLED="${ROUTER_TESTS_ENABLED:-true}" TMPDIR="${TMPDIR:-"/tmp"}" -ETCD_DATA_DIR=$(mktemp -d ${TMPDIR}/openshift.local.etcd.XXXX) -VOLUME_DIR=$(mktemp -d ${TMPDIR}/openshift.local.volumes.XXXX) -CERT_DIR=$(mktemp -d ${TMPDIR}/openshift.local.certificates.XXXX) -LOG_DIR="${LOG_DIR:-$(mktemp -d ${TMPDIR}/openshift.local.logs.XXXX)}" -ARTIFACT_DIR="${ARTIFACT_DIR:-$(mktemp -d ${TMPDIR}/openshift.local.artifacts.XXXX)}" +BASETMPDIR="${BASETMPDIR:-$(mktemp -d ${TMPDIR}/openshift-e2e.XXXX)}" +ETCD_DATA_DIR="${BASETMPDIR}/etcd" +VOLUME_DIR="${BASETMPDIR}/volumes" +CERT_DIR="${BASETMPDIR}/certs" +LOG_DIR="${LOG_DIR:-${BASETMPDIR}/logs}" +ARTIFACT_DIR="${ARTIFACT_DIR:-${BASETMPDIR}/artifacts}" mkdir -p $LOG_DIR mkdir -p $ARTIFACT_DIR API_PORT="${API_PORT:-8443}" @@ -135,37 +136,38 @@ trap teardown EXIT SIGINT # Setup stop_openshift_server echo "[INFO] `openshift version`" -echo "[INFO] Server logs will be at: $LOG_DIR/openshift.log" -echo "[INFO] Test artifacts will be in: $ARTIFACT_DIR" -echo "[INFO] Volumes dir is: $VOLUME_DIR" -echo "[INFO] Certs dir is: $CERT_DIR" +echo "[INFO] Server logs will be at: ${LOG_DIR}/openshift.log" +echo "[INFO] Test artifacts will be in: ${ARTIFACT_DIR}" +echo "[INFO] Volumes dir is: ${VOLUME_DIR}" +echo "[INFO] Certs dir is: ${CERT_DIR}" # Start All-in-one server and wait for health # Specify the scheme and port for the master, but let the IP auto-discover echo "[INFO] Starting OpenShift server" -sudo env "PATH=$PATH" openshift start --listen=$API_SCHEME://0.0.0.0:$API_PORT --volume-dir="${VOLUME_DIR}" --etcd-dir="${ETCD_DATA_DIR}" --cert-dir="${CERT_DIR}" --loglevel=4 &> "${LOG_DIR}/openshift.log" & +sudo env "PATH=${PATH}" openshift start --listen="${API_SCHEME}://0.0.0.0:${API_PORT}" --hostname="127.0.0.1" --volume-dir="${VOLUME_DIR}" --etcd-dir="${ETCD_DATA_DIR}" --cert-dir="${CERT_DIR}" --loglevel=4 &> "${LOG_DIR}/openshift.log" & OS_PID=$! -if [[ "$API_SCHEME" == "https" ]]; then - export CURL_CA_BUNDLE="$CERT_DIR/master/root.crt" +if [[ "${API_SCHEME}" == "https" ]]; then + export CURL_CA_BUNDLE="${CERT_DIR}/master/root.crt" fi -wait_for_url "$KUBELET_SCHEME://$API_HOST:$KUBELET_PORT/healthz" "[INFO] kubelet: " 1 30 -wait_for_url "$API_SCHEME://$API_HOST:$API_PORT/healthz" "[INFO] apiserver: " +wait_for_url "${KUBELET_SCHEME}://${API_HOST}:${KUBELET_PORT}/healthz" "[INFO] kubelet: " 0.5 60 +wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/healthz" "[INFO] apiserver: " 0.5 60 +wait_for_url "${API_SCHEME}://${API_HOST}:${API_PORT}/api/v1beta1/minions/127.0.0.1" "[INFO] apiserver(minions): " 0.2 60 # Set KUBERNETES_MASTER for osc -export KUBERNETES_MASTER=$API_SCHEME://$API_HOST:$API_PORT -if [[ "$API_SCHEME" == "https" ]]; then +export KUBERNETES_MASTER="${API_SCHEME}://${API_HOST}:${API_PORT}" +if [[ "${API_SCHEME}" == "https" ]]; then # Read client cert data in to send to containerized components - sudo chmod 644 $CERT_DIR/openshift-client/* - OPENSHIFT_CA_DATA=$(<$CERT_DIR/openshift-client/root.crt) - OPENSHIFT_CERT_DATA=$(<$CERT_DIR/openshift-client/cert.crt) - OPENSHIFT_KEY_DATA=$(<$CERT_DIR/openshift-client/key.key) - - # Make osc use $CERT_DIR/admin/.kubeconfig, and ignore anything in the running user's $HOME dir - sudo chmod 644 $CERT_DIR/admin/* - export HOME=$CERT_DIR/admin - export KUBECONFIG=$CERT_DIR/admin/.kubeconfig + sudo chmod -R a+rX "${CERT_DIR}/openshift-client/" + OPENSHIFT_CA_DATA="$(cat "${CERT_DIR}/openshift-client/root.crt")" + OPENSHIFT_CERT_DATA="$(cat "${CERT_DIR}/openshift-client/cert.crt")" + OPENSHIFT_KEY_DATA="$(cat "${CERT_DIR}/openshift-client/key.key")" + + # Make osc use ${CERT_DIR}/admin/.kubeconfig, and ignore anything in the running user's $HOME dir + sudo chmod -R a+rwX "${CERT_DIR}/admin/" + export HOME="${CERT_DIR}/admin" + export KUBECONFIG="${CERT_DIR}/admin/.kubeconfig" echo "[INFO] To debug: export KUBECONFIG=$KUBECONFIG" else OPENSHIFT_CA_DATA="" From 5193072c5cd77d348dfbb6bf830e308d1f53ada3 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Thu, 22 Jan 2015 16:48:43 -0500 Subject: [PATCH 15/18] WIP - Template updates --- .../application-template-custombuild.json | 23 +++++++++++-------- .../application-template-dockerbuild.json | 23 +++++++++++-------- .../application-template-stibuild.json | 17 +++++++------- .../sample-app/docker-registry-template.json | 1 - 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/examples/sample-app/application-template-custombuild.json b/examples/sample-app/application-template-custombuild.json index 42cb1aa8a293..9b09a7acd72a 100644 --- a/examples/sample-app/application-template-custombuild.json +++ b/examples/sample-app/application-template-custombuild.json @@ -1,7 +1,7 @@ { "metadata":{ "name": "ruby-helloworld-sample", - }, + }, "kind": "Template", "apiVersion": "v1beta1", "description": "This example shows how to create a simple ruby application in openshift origin v3", @@ -44,10 +44,9 @@ { "metadata":{ "name": "origin-ruby-sample", - }, + }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/custom/origin-ruby-sample", "labels": { "name": "origin-ruby-sample" } @@ -58,7 +57,6 @@ }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/openshift/origin-custom-docker-builder", "labels": { "name": "custom-docker-builder" } @@ -85,8 +83,10 @@ { "type": "imageChange", "imageChange": { - "image": "172.30.17.3:5001/openshift/origin-custom-docker-builder", - "imageRepositoryRef": { "name": "origin-custom-docker-builder"}, + "image": "openshift/origin-custom-docker-builder", + "from": { + "name": "origin-custom-docker-builder" + }, "tag":"latest" } } @@ -107,8 +107,9 @@ } }, "output": { - "imageTag": "custom/origin-ruby-sample:latest", - "registry": "172.30.17.3:5001" + "to": { + "name": "origin-ruby-sample" + } }, }, "labels": { @@ -129,7 +130,9 @@ "containerNames": [ "ruby-helloworld" ], - "repositoryName": "172.30.17.3:5001/custom/origin-ruby-sample", + "from": { + "name": "origin-ruby-sample" + }, "tag": "latest" } } @@ -150,7 +153,7 @@ "containers": [ { "name": "ruby-helloworld", - "image": "172.30.17.3:5001/custom/origin-ruby-sample", + "image": "origin-ruby-sample", "env": [ { "name": "ADMIN_USERNAME", diff --git a/examples/sample-app/application-template-dockerbuild.json b/examples/sample-app/application-template-dockerbuild.json index 71eda96a31e2..23ccf0382644 100644 --- a/examples/sample-app/application-template-dockerbuild.json +++ b/examples/sample-app/application-template-dockerbuild.json @@ -1,7 +1,7 @@ { "metadata":{ "name": "ruby-helloworld-sample", - }, + }, "kind": "Template", "apiVersion": "v1beta1", "description": "This example shows how to create a simple ruby application in openshift origin v3", @@ -44,10 +44,9 @@ { "metadata":{ "name": "origin-ruby-sample", - }, + }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/docker/origin-ruby-sample", "labels": { "name": "origin-ruby-sample" } @@ -58,7 +57,6 @@ }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/openshift/ruby-20-centos", "labels": { "name": "ruby-20-centos" } @@ -85,8 +83,10 @@ { "type": "imageChange", "imageChange": { - "image": "172.30.17.3:5001/openshift/ruby-20-centos", - "imageRepositoryRef": { "name": "ruby-20-centos"}, + "image": "openshift/ruby-20-centos", + "from": { + "name": "ruby-20-centos" + }, "tag":"latest" } } @@ -102,8 +102,9 @@ "type": "Docker" }, "output": { - "imageTag": "docker/origin-ruby-sample:latest", - "registry": "172.30.17.3:5001" + "to": { + "name": "origin-ruby-sample" + }, }, }, "labels": { @@ -124,7 +125,9 @@ "containerNames": [ "ruby-helloworld" ], - "repositoryName": "172.30.17.3:5001/docker/origin-ruby-sample", + "from": { + "name": "origin-ruby-sample" + }, "tag": "latest" } } @@ -145,7 +148,7 @@ "containers": [ { "name": "ruby-helloworld", - "image": "172.30.17.3:5001/docker/origin-ruby-sample", + "image": "origin-ruby-sample", "env": [ { "name": "ADMIN_USERNAME", diff --git a/examples/sample-app/application-template-stibuild.json b/examples/sample-app/application-template-stibuild.json index c539b31f2222..58ddbf0e2d30 100644 --- a/examples/sample-app/application-template-stibuild.json +++ b/examples/sample-app/application-template-stibuild.json @@ -47,7 +47,6 @@ }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/test/origin-ruby-sample", "labels": { "name": "origin-ruby-sample" } @@ -58,7 +57,6 @@ }, "kind": "ImageRepository", "apiVersion": "v1beta1", - "dockerImageRepository": "172.30.17.3:5001/openshift/ruby-20-centos", "labels": { "name": "ruby-20-centos" } @@ -85,8 +83,8 @@ { "type": "imageChange", "imageChange": { - "image": "172.30.17.3:5001/openshift/ruby-20-centos", - "imageRepositoryRef": { "name": "ruby-20-centos"}, + "image": "openshift/ruby-20-centos", + "from": { "name": "ruby-20-centos"}, "tag":"latest" } } @@ -106,8 +104,9 @@ } }, "output": { - "imageTag": "test/origin-ruby-sample:latest", - "registry": "172.30.17.3:5001" + "to": { + "name": "origin-ruby-sample" + } }, }, "labels": { @@ -128,7 +127,9 @@ "containerNames": [ "ruby-helloworld" ], - "repositoryName": "172.30.17.3:5001/test/origin-ruby-sample", + "from": { + "name": "origin-ruby-sample" + }, "tag": "latest" } } @@ -149,7 +150,7 @@ "containers": [ { "name": "ruby-helloworld", - "image": "172.30.17.3:5001/test/origin-ruby-sample", + "image": "origin-ruby-sample", "env": [ { "name": "ADMIN_USERNAME", diff --git a/examples/sample-app/docker-registry-template.json b/examples/sample-app/docker-registry-template.json index 2c2b8d419933..df8e965ccee4 100644 --- a/examples/sample-app/docker-registry-template.json +++ b/examples/sample-app/docker-registry-template.json @@ -33,7 +33,6 @@ "creationTimestamp":null, "id":"docker-registry", "kind":"Service", - "portalIp": "172.30.17.3", "port":5001, "containerPort":5000, "selector":{ From 991da56ef27f94bba530b546b65beb76234d8d0d Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 23 Jan 2015 11:30:05 -0500 Subject: [PATCH 16/18] Crash on Panic when ENV var set --- cmd/openshift/openshift.go | 3 +++ hack/test-cmd.sh | 2 ++ hack/test-end-to-end.sh | 1 + hack/test-go.sh | 2 ++ pkg/cmd/util/serviceability/panic.go | 16 ++++++++++++++++ 5 files changed, 24 insertions(+) create mode 100644 pkg/cmd/util/serviceability/panic.go diff --git a/cmd/openshift/openshift.go b/cmd/openshift/openshift.go index 949911b1c8e1..09e548d7b4a6 100644 --- a/cmd/openshift/openshift.go +++ b/cmd/openshift/openshift.go @@ -5,9 +5,12 @@ import ( "path/filepath" "github.com/openshift/origin/pkg/cmd/openshift" + "github.com/openshift/origin/pkg/cmd/util/serviceability" ) func main() { + serviceability.BehaviorOnPanic(os.Getenv("OPENSHIFT_ON_PANIC")) + basename := filepath.Base(os.Args[0]) command := openshift.CommandFor(basename) if err := command.Execute(); err != nil { diff --git a/hack/test-cmd.sh b/hack/test-cmd.sh index 943eb3d0833f..75be28ae32e0 100755 --- a/hack/test-cmd.sh +++ b/hack/test-cmd.sh @@ -25,6 +25,8 @@ trap cleanup EXIT SIGINT set -e +export OPENSHIFT_ON_PANIC=crash + USE_LOCAL_IMAGES=${USE_LOCAL_IMAGES:-true} ETCD_HOST=${ETCD_HOST:-127.0.0.1} diff --git a/hack/test-end-to-end.sh b/hack/test-end-to-end.sh index 28619f41691b..e7fcd61a0aaa 100755 --- a/hack/test-end-to-end.sh +++ b/hack/test-end-to-end.sh @@ -25,6 +25,7 @@ source "${OS_ROOT}/hack/util.sh" echo "[INFO] Starting end-to-end test" +export OPENSHIFT_ON_PANIC=crash USE_LOCAL_IMAGES="${USE_LOCAL_IMAGES:-true}" ROUTER_TESTS_ENABLED="${ROUTER_TESTS_ENABLED:-true}" diff --git a/hack/test-go.sh b/hack/test-go.sh index 4f07c3eb81eb..55f79c81145b 100755 --- a/hack/test-go.sh +++ b/hack/test-go.sh @@ -47,6 +47,8 @@ fi OUTPUT_COVERAGE=${OUTPUT_COVERAGE:-""} +export OPENSHIFT_ON_PANIC=crash + if [[ -n "${KUBE_COVER}" && -n "${OUTPUT_COVERAGE}" ]]; then # Iterate over packages to run coverage test_packages=( $test_packages ) diff --git a/pkg/cmd/util/serviceability/panic.go b/pkg/cmd/util/serviceability/panic.go new file mode 100644 index 000000000000..1e35ed33acdb --- /dev/null +++ b/pkg/cmd/util/serviceability/panic.go @@ -0,0 +1,16 @@ +package serviceability + +import ( + "github.com/golang/glog" + + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" +) + +// BehaviorOnPanic is a helper for setting the crash mode of OpenShift when a panic is caught. +func BehaviorOnPanic(mode string) { + switch mode { + case "crash": + glog.V(4).Infof("OpenShift will terminate as soon as a panic occurs.") + util.ReallyCrash = true + } +} From 4b207900809a1f38b28998ef688b9bced5d0ea6e Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 23 Jan 2015 11:30:19 -0500 Subject: [PATCH 17/18] Disable race detection on test-integration because of #731 --- hack/test-integration.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hack/test-integration.sh b/hack/test-integration.sh index 28a05d45eaf7..9d5d12451b9e 100755 --- a/hack/test-integration.sh +++ b/hack/test-integration.sh @@ -22,4 +22,5 @@ trap cleanup EXIT SIGINT echo echo Integration test cases ... echo -KUBE_RACE="${KUBE_RACE:--race}" KUBE_COVER=" " "${OS_ROOT}/hack/test-go.sh" test/integration -tags 'integration no-docker' "${@:1}" +# TODO: race is disabled because of origin #731 +KUBE_RACE="${KUBE_RACE:-}" KUBE_COVER=" " "${OS_ROOT}/hack/test-go.sh" test/integration -tags 'integration no-docker' "${@:1}" From b176c34a4b30e0c982ff4651b48651bf68602cbf Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Fri, 23 Jan 2015 11:30:34 -0500 Subject: [PATCH 18/18] Review comments 2 - Pass Codec through and return error on setBuildEnv --- pkg/build/controller/strategy/custom.go | 12 +++++-- pkg/build/controller/strategy/custom_test.go | 1 + pkg/build/controller/strategy/docker.go | 8 +++-- pkg/build/controller/strategy/docker_test.go | 1 + pkg/build/controller/strategy/sti.go | 8 +++-- pkg/build/controller/strategy/sti_test.go | 1 + pkg/build/controller/strategy/util.go | 13 +++++--- pkg/build/controller/strategy/util_test.go | 35 ++++++++++++++++++++ pkg/cmd/server/origin/master.go | 6 ++++ 9 files changed, 73 insertions(+), 12 deletions(-) diff --git a/pkg/build/controller/strategy/custom.go b/pkg/build/controller/strategy/custom.go index fd687e27a7fe..8c0bd3377e21 100644 --- a/pkg/build/controller/strategy/custom.go +++ b/pkg/build/controller/strategy/custom.go @@ -4,20 +4,24 @@ import ( "errors" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/golang/glog" - "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) // CustomBuildStrategy creates a build using a custom builder image. type CustomBuildStrategy struct { UseLocalImages bool + // Codec is the codec to use for encoding the output pod. + // IMPORTANT: This may break backwards compatibility when + // it changes. + Codec runtime.Codec } // CreateBuildPod creates the pod to be used for the Custom build func (bs *CustomBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - data, err := v1beta1.Codec.Encode(build) + data, err := bs.Codec.Encode(build) if err != nil { return nil, err } @@ -61,7 +65,9 @@ func (bs *CustomBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, }, } - setupBuildEnv(build, pod) + if err := setupBuildEnv(build, pod); err != nil { + return nil, err + } if bs.UseLocalImages { pod.Spec.Containers[0].ImagePullPolicy = kapi.PullIfNotPresent diff --git a/pkg/build/controller/strategy/custom_test.go b/pkg/build/controller/strategy/custom_test.go index f30ffae1cf3f..30935df97bc6 100644 --- a/pkg/build/controller/strategy/custom_test.go +++ b/pkg/build/controller/strategy/custom_test.go @@ -12,6 +12,7 @@ import ( func TestCustomCreateBuildPod(t *testing.T) { strategy := CustomBuildStrategy{ UseLocalImages: true, + Codec: v1beta1.Codec, } expectedBad := mockCustomBuild() diff --git a/pkg/build/controller/strategy/docker.go b/pkg/build/controller/strategy/docker.go index 974eedb15f6f..14fe280f762d 100644 --- a/pkg/build/controller/strategy/docker.go +++ b/pkg/build/controller/strategy/docker.go @@ -2,8 +2,8 @@ package strategy import ( kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -11,12 +11,16 @@ import ( type DockerBuildStrategy struct { Image string UseLocalImages bool + // Codec is the codec to use for encoding the output pod. + // IMPORTANT: This may break backwards compatibility when + // it changes. + Codec runtime.Codec } // CreateBuildPod creates the pod to be used for the Docker build // TODO: Make the Pod definition configurable func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - data, err := v1beta1.Codec.Encode(build) + data, err := bs.Codec.Encode(build) if err != nil { return nil, err } diff --git a/pkg/build/controller/strategy/docker_test.go b/pkg/build/controller/strategy/docker_test.go index 8378c7a690d2..4666c12cf3d6 100644 --- a/pkg/build/controller/strategy/docker_test.go +++ b/pkg/build/controller/strategy/docker_test.go @@ -13,6 +13,7 @@ func TestDockerCreateBuildPod(t *testing.T) { strategy := DockerBuildStrategy{ Image: "docker-test-image", UseLocalImages: true, + Codec: v1beta1.Codec, } expected := mockDockerBuild() diff --git a/pkg/build/controller/strategy/sti.go b/pkg/build/controller/strategy/sti.go index e5731c037423..433d7aa8553f 100644 --- a/pkg/build/controller/strategy/sti.go +++ b/pkg/build/controller/strategy/sti.go @@ -4,8 +4,8 @@ import ( "io/ioutil" kapi "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - "github.com/openshift/origin/pkg/api/v1beta1" buildapi "github.com/openshift/origin/pkg/build/api" ) @@ -14,6 +14,10 @@ type STIBuildStrategy struct { Image string TempDirectoryCreator TempDirectoryCreator UseLocalImages bool + // Codec is the codec to use for encoding the output pod. + // IMPORTANT: This may break backwards compatibility when + // it changes. + Codec runtime.Codec } type TempDirectoryCreator interface { @@ -31,7 +35,7 @@ var STITempDirectoryCreator = &tempDirectoryCreator{} // CreateBuildPod creates a pod that will execute the STI build // TODO: Make the Pod definition configurable func (bs *STIBuildStrategy) CreateBuildPod(build *buildapi.Build) (*kapi.Pod, error) { - data, err := v1beta1.Codec.Encode(build) + data, err := bs.Codec.Encode(build) if err != nil { return nil, err } diff --git a/pkg/build/controller/strategy/sti_test.go b/pkg/build/controller/strategy/sti_test.go index a00560509f59..6bc0e66b212d 100644 --- a/pkg/build/controller/strategy/sti_test.go +++ b/pkg/build/controller/strategy/sti_test.go @@ -20,6 +20,7 @@ func TestSTICreateBuildPod(t *testing.T) { Image: "sti-test-image", TempDirectoryCreator: &FakeTempDirCreator{}, UseLocalImages: true, + Codec: v1beta1.Codec, } expected := mockSTIBuild() diff --git a/pkg/build/controller/strategy/util.go b/pkg/build/controller/strategy/util.go index 28d15ad91f65..d98451392a19 100644 --- a/pkg/build/controller/strategy/util.go +++ b/pkg/build/controller/strategy/util.go @@ -66,7 +66,7 @@ func setupDockerConfig(podSpec *kapi.Pod) { // setupBuildEnv injects human-friendly environment variables which provides // useful information about the current build. -func setupBuildEnv(build *buildapi.Build, pod *kapi.Pod) { +func setupBuildEnv(build *buildapi.Build, pod *kapi.Pod) error { vars := []kapi.EnvVar{} switch build.Parameters.Source.Type { @@ -77,13 +77,16 @@ func setupBuildEnv(build *buildapi.Build, pod *kapi.Pod) { // Do nothing for unknown source types } - if registry, namespace, name, tag, err := imageapi.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference); err == nil { - outputImage := imageapi.JoinDockerPullSpec("", namespace, name, tag) - vars = append(vars, kapi.EnvVar{"OUTPUT_IMAGE", outputImage}) - vars = append(vars, kapi.EnvVar{"OUTPUT_REGISTRY", registry}) + registry, namespace, name, tag, err := imageapi.SplitDockerPullSpec(build.Parameters.Output.DockerImageReference) + if err != nil { + return err } + outputImage := imageapi.JoinDockerPullSpec("", namespace, name, tag) + vars = append(vars, kapi.EnvVar{"OUTPUT_IMAGE", outputImage}) + vars = append(vars, kapi.EnvVar{"OUTPUT_REGISTRY", registry}) if len(pod.Spec.Containers) > 0 { pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, vars...) } + return nil } diff --git a/pkg/build/controller/strategy/util_test.go b/pkg/build/controller/strategy/util_test.go index 56430ee2e597..203b53c5df16 100644 --- a/pkg/build/controller/strategy/util_test.go +++ b/pkg/build/controller/strategy/util_test.go @@ -51,3 +51,38 @@ func TestSetupDockerSocketHostSocket(t *testing.T) { t.Error("Expected privileged to be false") } } + +func TestSetupBuildEnvFails(t *testing.T) { + build := mockCustomBuild() + containerEnv := []kapi.EnvVar{ + {Name: "BUILD", Value: ""}, + {Name: "SOURCE_REPOSITORY", Value: build.Parameters.Source.Git.URI}, + } + pod := &kapi.Pod{ + ObjectMeta: kapi.ObjectMeta{ + Name: build.PodName, + }, + Spec: kapi.PodSpec{ + Containers: []kapi.Container{ + { + Name: "custom-build", + Image: build.Parameters.Strategy.CustomStrategy.Image, + Env: containerEnv, + // TODO: run unprivileged https://github.com/openshift/origin/issues/662 + Privileged: true, + }, + }, + RestartPolicy: kapi.RestartPolicy{ + Never: &kapi.RestartPolicyNever{}, + }, + }, + } + if err := setupBuildEnv(build, pod); err != nil { + t.Errorf("unexpected error: %v", err) + } + + build.Parameters.Output.DockerImageReference = "" + if err := setupBuildEnv(build, pod); err == nil { + t.Errorf("unexpected non-error: %v", err) + } +} diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index 8c479c87e599..2351851f9974 100644 --- a/pkg/cmd/server/origin/master.go +++ b/pkg/cmd/server/origin/master.go @@ -470,14 +470,20 @@ func (c *MasterConfig) RunBuildController() { DockerBuildStrategy: &buildstrategy.DockerBuildStrategy{ Image: dockerImage, UseLocalImages: useLocalImages, + // TODO: this will be set to --storage-version (the internal schema we use) + Codec: v1beta1.Codec, }, STIBuildStrategy: &buildstrategy.STIBuildStrategy{ Image: stiImage, TempDirectoryCreator: buildstrategy.STITempDirectoryCreator, UseLocalImages: useLocalImages, + // TODO: this will be set to --storage-version (the internal schema we use) + Codec: v1beta1.Codec, }, CustomBuildStrategy: &buildstrategy.CustomBuildStrategy{ UseLocalImages: useLocalImages, + // TODO: this will be set to --storage-version (the internal schema we use) + Codec: v1beta1.Codec, }, }