reject build requests for binary builds if not providing binary inputs

Author: bparees

pkg/build/api/validation/validation.go

@@ -142,7 +142,7 @@ func validateCommonSpec(spec *buildapi.CommonSpec, fldPath *field.Path) field.Er
 	s := spec.Strategy
 
 	if s.DockerStrategy != nil && s.JenkinsPipelineStrategy == nil && spec.Source.Git == nil && spec.Source.Binary == nil && spec.Source.Dockerfile == nil && spec.Source.Images == nil {
-		allErrs = append(allErrs, field.Invalid(fldPath.Child("source"), spec.Source, "must provide a value for at least one of source, binary, images, or dockerfile"))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("source"), "", "must provide a value for at least one source input(git, binary, dockerfile, images)."))
 	}
 
 	allErrs = append(allErrs,
@@ -202,7 +202,6 @@ func validateSource(input *buildapi.BuildSource, isCustomStrategy, isDockerStrat
 			allErrs = append(allErrs, validateImageSource(image, fldPath.Child("images").Index(i))...)
 		}
 	}
-
 	if isJenkinsPipelineStrategyFromRepo && input.Git == nil {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("git"), "", "must be set when using Jenkins Pipeline strategy with Jenkinsfile from a git repo"))
 	}

pkg/build/api/validation/validation_test.go

@@ -46,7 +46,7 @@ func TestBuildValidationSuccess(t *testing.T) {
 
 func checkDockerStrategyEmptySourceError(result field.ErrorList) bool {
 	for _, err := range result {
-		if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Field, "spec.source") && strings.Contains(err.Detail, "must provide a value for at least one of source, binary, images, or dockerfile") {
+		if err.Type == field.ErrorTypeInvalid && strings.Contains(err.Field, "spec.source") && strings.Contains(err.Detail, "must provide a value for at least one source input(git, binary, dockerfile, images).") {
 			return true
 		}
 	}

pkg/build/controller/image_change_controller_test.go

@@ -302,12 +302,16 @@ func (m *mockBuildConfigUpdater) Update(buildcfg *buildapi.BuildConfig) error {
 }
 
 func mockBuildConfig(baseImage, triggerImage, repoName, repoTag string) *buildapi.BuildConfig {
+	dockerfile := "FROM foo"
 	return &buildapi.BuildConfig{
 		ObjectMeta: kapi.ObjectMeta{
 			Name: "testBuildCfg",
 		},
 		Spec: buildapi.BuildConfigSpec{
 			CommonSpec: buildapi.CommonSpec{
+				Source: buildapi.BuildSource{
+					Dockerfile: &dockerfile,
+				},
 				Strategy: buildapi.BuildStrategy{
 					DockerStrategy: &buildapi.DockerBuildStrategy{
 						From: &kapi.ObjectReference{

pkg/build/generator/generator.go

@@ -218,9 +218,8 @@ func (g *BuildGenerator) Instantiate(ctx kapi.Context, request *buildapi.BuildRe
 	if err != nil {
 		return nil, err
 	}
-
 	if buildutil.IsPaused(bc) {
-		return nil, errors.NewInternalError(&GeneratorFatalError{fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", bc.Namespace, bc.Name)})
+		return nil, errors.NewBadRequest(fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", bc.Namespace, bc.Name))
 	}
 
 	if err := g.checkLastVersion(bc, request.LastVersion); err != nil {
@@ -333,7 +332,6 @@ func (g *BuildGenerator) Clone(ctx kapi.Context, request *buildapi.BuildRequest)
 		if err != nil && !errors.IsNotFound(err) {
 			return nil, err
 		}
-
 		if buildutil.IsPaused(buildConfig) {
 			return nil, errors.NewInternalError(&GeneratorFatalError{fmt.Sprintf("can't instantiate from BuildConfig %s/%s: BuildConfig is paused", buildConfig.Namespace, buildConfig.Name)})
 		}
@@ -366,7 +364,6 @@ func (g *BuildGenerator) createBuild(ctx kapi.Context, build *buildapi.Build) (*
 		return nil, errors.NewConflict(buildapi.Resource("build"), build.Namespace, fmt.Errorf("Build.Namespace does not match the provided context"))
 	}
 	kapi.FillObjectMetaSystemFields(ctx, &build.ObjectMeta)
-
 	err := g.Client.CreateBuild(ctx, build)
 	if err != nil {
 		return nil, err
@@ -416,13 +413,15 @@ func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.
 			},
 		},
 	}
-
 	if binary != nil {
 		build.Spec.Source.Git = nil
 		build.Spec.Source.Binary = binary
 		if build.Spec.Source.Dockerfile != nil && binary.AsFile == "Dockerfile" {
 			build.Spec.Source.Dockerfile = nil
 		}
+	} else {
+		// must explicitly set this because we copied the source values from the buildconfig.
+		build.Spec.Source.Binary = nil
 	}
 
 	build.Name = getNextBuildName(bc)
@@ -698,6 +697,8 @@ func generateBuildFromBuild(build *buildapi.Build, buildConfig *buildapi.BuildCo
 			Config: buildCopy.Status.Config,
 		},
 	}
+	// TODO remove/update this when we support cloning binary builds
+	newBuild.Spec.Source.Binary = nil
 	if newBuild.Annotations == nil {
 		newBuild.Annotations = make(map[string]string)
 	}

pkg/build/generator/generator_test.go

@@ -48,6 +48,25 @@ func TestInstantiate(t *testing.T) {
 	}
 }
 
+func TestInstantiateBinary(t *testing.T) {
+	generator := mockBuildGenerator()
+	build, err := generator.Instantiate(kapi.NewDefaultContext(), &buildapi.BuildRequest{Binary: &buildapi.BinaryBuildSource{}})
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	if build.Spec.Source.Binary == nil {
+		t.Errorf("build should have a binary source value, has nil")
+	}
+	build, err = generator.Clone(kapi.NewDefaultContext(), &buildapi.BuildRequest{Binary: &buildapi.BinaryBuildSource{}})
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	// TODO: we should enable this flow.
+	if build.Spec.Source.Binary != nil {
+		t.Errorf("build should not have a binary source value, has %v", build.Spec.Source.Binary)
+	}
+}
+
 // TODO(agoldste): I'm not sure the intent of this test. Using the previous logic for
 // the generator, which would try to update the build config before creating
 // the build, I can see why the UpdateBuildConfigFunc is set up to return an
@@ -83,6 +102,7 @@ func TestInstantiateRetry(t *testing.T) {
 */
 
 func TestInstantiateDeletingError(t *testing.T) {
+	source := mocks.MockSource()
 	generator := BuildGenerator{Client: Client{
 		GetBuildConfigFunc: func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
 			bc := &buildapi.BuildConfig{
@@ -91,11 +111,31 @@ func TestInstantiateDeletingError(t *testing.T) {
 						buildapi.BuildConfigPausedAnnotation: "true",
 					},
 				},
+				Spec: buildapi.BuildConfigSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Source: source,
+						Revision: &buildapi.SourceRevision{
+							Git: &buildapi.GitSourceRevision{
+								Commit: "1234",
+							},
+						},
+					},
+				},
 			}
 			return bc, nil
 		},
 		GetBuildFunc: func(ctx kapi.Context, name string) (*buildapi.Build, error) {
 			build := &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Source: source,
+						Revision: &buildapi.SourceRevision{
+							Git: &buildapi.GitSourceRevision{
+								Commit: "1234",
+							},
+						},
+					},
+				},
 				Status: buildapi.BuildStatus{
 					Config: &kapi.ObjectReference{
 						Name: "buildconfig",
@@ -115,6 +155,61 @@ func TestInstantiateDeletingError(t *testing.T) {
 	}
 }
 
+// TestInstantiateBinaryClear ensures that when instantiating or cloning from a buildconfig/build
+// that has a binary source value, the resulting build does not have a binary source value
+// (because the request did not include one)
+func TestInstantiateBinaryRemoved(t *testing.T) {
+	generator := mockBuildGenerator()
+	client := generator.Client.(Client)
+	client.GetBuildConfigFunc = func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
+		bc := &buildapi.BuildConfig{
+			ObjectMeta: kapi.ObjectMeta{
+				Annotations: map[string]string{},
+			},
+			Spec: buildapi.BuildConfigSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Binary: &buildapi.BinaryBuildSource{},
+					},
+				},
+			},
+		}
+		return bc, nil
+	}
+	client.GetBuildFunc = func(ctx kapi.Context, name string) (*buildapi.Build, error) {
+		build := &buildapi.Build{
+			Spec: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Binary: &buildapi.BinaryBuildSource{},
+					},
+				},
+			},
+			Status: buildapi.BuildStatus{
+				Config: &kapi.ObjectReference{
+					Name: "buildconfig",
+				},
+			},
+		}
+		return build, nil
+	}
+
+	build, err := generator.Instantiate(kapi.NewDefaultContext(), &buildapi.BuildRequest{})
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	if build.Spec.Source.Binary != nil {
+		t.Errorf("build should not have a binary source value, has %v", build.Spec.Source.Binary)
+	}
+	build, err = generator.Clone(kapi.NewDefaultContext(), &buildapi.BuildRequest{})
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	if build.Spec.Source.Binary != nil {
+		t.Errorf("build should not have a binary source value, has %v", build.Spec.Source.Binary)
+	}
+}
+
 func TestInstantiateGetBuildConfigError(t *testing.T) {
 	generator := BuildGenerator{Client: Client{
 		GetBuildConfigFunc: func(ctx kapi.Context, name string) (*buildapi.BuildConfig, error) {
@@ -240,6 +335,7 @@ func TestInstantiateWithImageTrigger(t *testing.T) {
 		},
 	}
 
+	source := mocks.MockSource()
 	for _, tc := range tests {
 		bc := &buildapi.BuildConfig{
 			Spec: buildapi.BuildConfigSpec{
@@ -252,6 +348,12 @@ func TestInstantiateWithImageTrigger(t *testing.T) {
 							},
 						},
 					},
+					Source: source,
+					Revision: &buildapi.SourceRevision{
+						Git: &buildapi.GitSourceRevision{
+							Commit: "1234",
+						},
+					},
 				},
 				Triggers: tc.triggers,
 			},

pkg/cmd/cli/cmd/startbuild.go

@@ -19,6 +19,7 @@ import (
 	"github.com/spf13/cobra"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	kerrors "k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/client/restclient"
 	kclientcmd "k8s.io/kubernetes/pkg/client/unversioned/clientcmd"
@@ -30,7 +31,7 @@ import (
 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 	"github.com/openshift/origin/pkg/generate/git"
-	"github.com/openshift/origin/pkg/util/errors"
+	oerrors "github.com/openshift/origin/pkg/util/errors"
 	"github.com/openshift/source-to-image/pkg/tar"
 )
 
@@ -165,6 +166,11 @@ func (o *StartBuildOptions) Complete(f *clientcmd.Factory, in io.Reader, out io.
 		return kcmdutil.UsageError(cmd, "Must pass a name of a build config or specify build name with '--from-build' flag")
 	}
 
+	if len(buildName) != 0 && (len(fromFile) != 0 || len(fromDir) != 0 || len(fromRepo) != 0) {
+		// TODO: we should support this, it should be possible to clone a build to run again with new uploaded artifacts.
+		// Doing so requires introducing a new clonebinary endpoint.
+		return kcmdutil.UsageError(cmd, "Cannot use '--from-build' flag with binary builds")
+	}
 	o.AsBinary = len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0
 
 	namespace, _, err := f.DefaultNamespace()
@@ -282,10 +288,16 @@ func (o *StartBuildOptions) Run() error {
 		}
 	case len(o.FromBuild) > 0:
 		if newBuild, err = o.Client.Builds(o.Namespace).Clone(request); err != nil {
+			if isInvalidSourceInputsError(err) {
+				return fmt.Errorf("Build %s/%s has no valid source inputs and '--from-build' cannot be used for binary builds", o.Namespace, o.Name)
+			}
 			return err
 		}
 	default:
 		if newBuild, err = o.Client.BuildConfigs(o.Namespace).Instantiate(request); err != nil {
+			if isInvalidSourceInputsError(err) {
+				return fmt.Errorf("Build configuration %s/%s has no valid source inputs, if this is a binary build you must specify one of '--from-dir', '--from-repo', or '--from-file'", o.Namespace, o.Name)
+			}
 			return err
 		}
 	}
@@ -327,7 +339,7 @@ func (o *StartBuildOptions) Run() error {
 				if err != nil {
 					// if --wait options is set, then retry the connection to build logs
 					// when we hit the timeout.
-					if o.WaitForComplete && errors.IsTimeoutErr(err) {
+					if o.WaitForComplete && oerrors.IsTimeoutErr(err) {
 						continue
 					}
 					fmt.Fprintf(o.ErrOut, "error getting logs: %v\n", err)
@@ -717,3 +729,14 @@ func WaitForBuildComplete(c osclient.BuildInterface, name string) error {
 		}
 	}
 }
+
+func isInvalidSourceInputsError(err error) bool {
+	if err != nil {
+		if statusErr, ok := err.(*kerrors.StatusError); ok {
+			if statusErr.ErrStatus.Reason == "Invalid" && strings.Contains(statusErr.ErrStatus.Message, "spec.source") {
+				return true
+			}
+		}
+	}
+	return false
+}

test/cmd/builds.sh

@@ -163,6 +163,9 @@ started=$(oc start-build ruby-sample-build-invalidtag)
 os::cmd::expect_success_and_text "oc describe build ${started}" 'centos/ruby-22-centos7$'
 frombuild=$(oc start-build --from-build="${started}")
 os::cmd::expect_success_and_text "oc describe build ${frombuild}" 'centos/ruby-22-centos7$'
+os::cmd::expect_failure_and_text "oc start-build ruby-sample-build-invalid-tag --from-dir=. --from-build=${started}" "Cannot use '--from-build' flag with binary builds"
+os::cmd::expect_failure_and_text "oc start-build ruby-sample-build-invalid-tag --from-file=. --from-build=${started}" "Cannot use '--from-build' flag with binary builds"
+os::cmd::expect_failure_and_text "oc start-build ruby-sample-build-invalid-tag --from-repo=. --from-build=${started}" "Cannot use '--from-build' flag with binary builds"
 echo "start-build: ok"
 os::test::junit::declare_suite_end
 

test/extended/builds/start.go

@@ -155,6 +155,21 @@ var _ = g.Describe("[builds][Slow] starting a build using CLI", func() {
 			}
 			o.Expect(err).NotTo(o.HaveOccurred())
 		})
+
+		g.It("should get an error if no --from-xxxx is specified", func() {
+			g.By("starting the build")
+			out, err := oc.Run("start-build").Args("sample-build").Output()
+			o.Expect(err).To(o.HaveOccurred())
+			o.Expect(out).To(o.ContainSubstring("has no valid source inputs"))
+		})
+
+		g.It("should get an error if no --from-xxxx is specified with --from-build", func() {
+			g.By("starting the build")
+			out, err := oc.Run("start-build").Args("sample-build", fmt.Sprintf("--from-build=%s", "sample-build-1")).Output()
+			o.Expect(err).To(o.HaveOccurred())
+			o.Expect(out).To(o.ContainSubstring("has no valid source inputs"))
+		})
+
 	})
 
 	g.Describe("cancelling build started by oc start-build --wait", func() {