Browse code

Cleanup output to builds, force validation, better errors

The main builders should be using an output stream passed in to them,
and should not be glog.Fatalfing. Allow builds to run on non-Linux
systems. Perform validation on the incoming build to handle bad input
and protect against malicious build injection. Cleanup the commands
slightly.

Clayton Coleman authored on 2016/05/07 10:46:48
Showing 5 changed files
... ...
@@ -2,6 +2,7 @@ package cmd
2 2
 
3 3
 import (
4 4
 	"fmt"
5
+	"io"
5 6
 	"io/ioutil"
6 7
 	"os"
7 8
 	"os/exec"
... ...
@@ -11,12 +12,15 @@ import (
11 11
 	"github.com/golang/glog"
12 12
 
13 13
 	kapi "k8s.io/kubernetes/pkg/api"
14
+	"k8s.io/kubernetes/pkg/api/errors"
15
+	"k8s.io/kubernetes/pkg/api/unversioned"
14 16
 	"k8s.io/kubernetes/pkg/client/restclient"
15 17
 	"k8s.io/kubernetes/pkg/runtime"
16 18
 
17 19
 	s2iapi "github.com/openshift/source-to-image/pkg/api"
18 20
 
19 21
 	"github.com/openshift/origin/pkg/build/api"
22
+	"github.com/openshift/origin/pkg/build/api/validation"
20 23
 	bld "github.com/openshift/origin/pkg/build/builder"
21 24
 	"github.com/openshift/origin/pkg/build/builder/cmd/scmauth"
22 25
 	"github.com/openshift/origin/pkg/client"
... ...
@@ -30,6 +34,7 @@ type builder interface {
30 30
 }
31 31
 
32 32
 type builderConfig struct {
33
+	out             io.Writer
33 34
 	build           *api.Build
34 35
 	sourceSecretDir string
35 36
 	dockerClient    *docker.Client
... ...
@@ -37,24 +42,30 @@ type builderConfig struct {
37 37
 	buildsClient    client.BuildInterface
38 38
 }
39 39
 
40
-func newBuilderConfigFromEnvironment() (*builderConfig, error) {
40
+func newBuilderConfigFromEnvironment(out io.Writer) (*builderConfig, error) {
41 41
 	cfg := &builderConfig{}
42 42
 	var err error
43 43
 
44
+	cfg.out = out
45
+
44 46
 	// build (BUILD)
45 47
 	buildStr := os.Getenv("BUILD")
46 48
 	glog.V(4).Infof("$BUILD env var is %s \n", buildStr)
47 49
 	cfg.build = &api.Build{}
48
-	if err = runtime.DecodeInto(kapi.Codecs.UniversalDecoder(), []byte(buildStr), cfg.build); err != nil {
50
+	if err := runtime.DecodeInto(kapi.Codecs.UniversalDecoder(), []byte(buildStr), cfg.build); err != nil {
49 51
 		return nil, fmt.Errorf("unable to parse build: %v", err)
50 52
 	}
53
+	if errs := validation.ValidateBuild(cfg.build); len(errs) > 0 {
54
+		return nil, errors.NewInvalid(unversioned.GroupKind{Kind: "Build"}, cfg.build.Name, errs)
55
+	}
56
+	glog.V(4).Infof("Build: %#v", cfg.build)
51 57
 
52 58
 	masterVersion := os.Getenv(api.OriginVersion)
53 59
 	thisVersion := version.Get().String()
54 60
 	if len(masterVersion) != 0 && masterVersion != thisVersion {
55
-		glog.Warningf("Master version %q does not match Builder image version %q", masterVersion, thisVersion)
61
+		fmt.Fprintf(cfg.out, "warning: OpenShift server version %q differs from this image %q\n", masterVersion, thisVersion)
56 62
 	} else {
57
-		glog.V(2).Infof("Master version %q, Builder version %q", masterVersion, thisVersion)
63
+		glog.V(4).Infof("Master version %q, Builder version %q", masterVersion, thisVersion)
58 64
 	}
59 65
 
60 66
 	// sourceSecretsDir (SOURCE_SECRET_PATH)
... ...
@@ -64,17 +75,17 @@ func newBuilderConfigFromEnvironment() (*builderConfig, error) {
64 64
 	// usually not set, defaults to docker socket
65 65
 	cfg.dockerClient, cfg.dockerEndpoint, err = dockerutil.NewHelper().GetClient()
66 66
 	if err != nil {
67
-		return nil, fmt.Errorf("error obtaining docker client: %v", err)
67
+		return nil, fmt.Errorf("no Docker configuration defined: %v", err)
68 68
 	}
69 69
 
70 70
 	// buildsClient (KUBERNETES_SERVICE_HOST, KUBERNETES_SERVICE_PORT)
71 71
 	clientConfig, err := restclient.InClusterConfig()
72 72
 	if err != nil {
73
-		return nil, fmt.Errorf("failed to get client config: %v", err)
73
+		return nil, fmt.Errorf("cannot connect to the server: %v", err)
74 74
 	}
75 75
 	osClient, err := client.New(clientConfig)
76 76
 	if err != nil {
77
-		return nil, fmt.Errorf("error obtaining OpenShift client: %v", err)
77
+		return nil, fmt.Errorf("failed to get client: %v", err)
78 78
 	}
79 79
 	cfg.buildsClient = osClient.Builds(cfg.build.Namespace)
80 80
 
... ...
@@ -82,10 +93,8 @@ func newBuilderConfigFromEnvironment() (*builderConfig, error) {
82 82
 }
83 83
 
84 84
 func (c *builderConfig) setupGitEnvironment() ([]string, error) {
85
-
86
-	gitSource := c.build.Spec.Source.Git
87
-
88 85
 	// For now, we only handle git. If not specified, we're done
86
+	gitSource := c.build.Spec.Source.Git
89 87
 	if gitSource == nil {
90 88
 		return []string{}, nil
91 89
 	}
... ...
@@ -131,7 +140,6 @@ func (c *builderConfig) setupGitEnvironment() ([]string, error) {
131 131
 
132 132
 // execute is responsible for running a build
133 133
 func (c *builderConfig) execute(b builder) error {
134
-
135 134
 	gitEnv, err := c.setupGitEnvironment()
136 135
 	if err != nil {
137 136
 		return err
... ...
@@ -142,14 +150,14 @@ func (c *builderConfig) execute(b builder) error {
142 142
 	if err != nil {
143 143
 		return fmt.Errorf("failed to retrieve cgroup limits: %v", err)
144 144
 	}
145
-	glog.V(2).Infof("Running build with cgroup limits: %#v", *cgLimits)
145
+	glog.V(4).Infof("Running build with cgroup limits: %#v", *cgLimits)
146 146
 
147 147
 	if err := b.Build(c.dockerClient, c.dockerEndpoint, c.buildsClient, c.build, gitClient, cgLimits); err != nil {
148 148
 		return fmt.Errorf("build error: %v", err)
149 149
 	}
150 150
 
151 151
 	if c.build.Spec.Output.To == nil || len(c.build.Spec.Output.To.Name) == 0 {
152
-		glog.Warning("Build does not have an Output defined, no output image was pushed to a registry.")
152
+		fmt.Fprintf(c.out, "Build complete, no image push requested\n")
153 153
 	}
154 154
 
155 155
 	return nil
... ...
@@ -194,23 +202,20 @@ func (s2iBuilder) Build(dockerClient bld.DockerClient, sock string, buildsClient
194 194
 	return bld.NewS2IBuilder(dockerClient, sock, buildsClient, build, gitClient, cgLimits).Build()
195 195
 }
196 196
 
197
-func runBuild(builder builder) {
198
-	cfg, err := newBuilderConfigFromEnvironment()
197
+func runBuild(out io.Writer, builder builder) error {
198
+	cfg, err := newBuilderConfigFromEnvironment(out)
199 199
 	if err != nil {
200
-		glog.Fatalf("Cannot setup builder configuration: %v", err)
201
-	}
202
-	err = cfg.execute(builder)
203
-	if err != nil {
204
-		glog.Fatalf("Error: %v", err)
200
+		return err
205 201
 	}
202
+	return cfg.execute(builder)
206 203
 }
207 204
 
208 205
 // RunDockerBuild creates a docker builder and runs its build
209
-func RunDockerBuild() {
210
-	runBuild(dockerBuilder{})
206
+func RunDockerBuild(out io.Writer) error {
207
+	return runBuild(out, dockerBuilder{})
211 208
 }
212 209
 
213 210
 // RunSTIBuild creates a STI builder and runs its build
214
-func RunSTIBuild() {
215
-	runBuild(s2iBuilder{})
211
+func RunSTIBuild(out io.Writer) error {
212
+	return runBuild(out, s2iBuilder{})
216 213
 }
... ...
@@ -217,7 +217,6 @@ func (t *basicAuthTransport) RoundTrip(req *http.Request) (*http.Response, error
217 217
 }
218 218
 
219 219
 func startProxy(dir string, sourceURL *url.URL, username, password string) (*url.URL, error) {
220
-
221 220
 	// Setup the targetURL of the proxy to be the host of the original URL
222 221
 	targetURL := &url.URL{
223 222
 		Scheme: sourceURL.Scheme,
... ...
@@ -71,6 +71,10 @@ func getDockerNetworkMode() s2iapi.DockerNetworkMode {
71 71
 func GetCGroupLimits() (*s2iapi.CGroupLimits, error) {
72 72
 	byteLimit, err := readInt64("/sys/fs/cgroup/memory/memory.limit_in_bytes")
73 73
 	if err != nil {
74
+		// for systems without cgroups builds should succeed
75
+		if _, err := os.Stat("/sys/fs/cgroup"); os.IsNotExist(err) {
76
+			return &s2iapi.CGroupLimits{}, nil
77
+		}
74 78
 		return nil, fmt.Errorf("cannot determine cgroup limits: %v", err)
75 79
 	}
76 80
 	// math.MaxInt64 seems to give cgroups trouble, this value is
... ...
@@ -3,6 +3,8 @@ package builder
3 3
 import (
4 4
 	"github.com/spf13/cobra"
5 5
 
6
+	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
7
+
6 8
 	"github.com/openshift/origin/pkg/build/builder/cmd"
7 9
 	"github.com/openshift/origin/pkg/version"
8 10
 )
... ...
@@ -28,7 +30,8 @@ func NewCommandSTIBuilder(name string) *cobra.Command {
28 28
 		Short: "Run a Source-to-Image build",
29 29
 		Long:  stiBuilderLong,
30 30
 		Run: func(c *cobra.Command, args []string) {
31
-			cmd.RunSTIBuild()
31
+			err := cmd.RunSTIBuild(c.Out())
32
+			kcmdutil.CheckErr(err)
32 33
 		},
33 34
 	}
34 35
 
... ...
@@ -43,7 +46,8 @@ func NewCommandDockerBuilder(name string) *cobra.Command {
43 43
 		Short: "Run a Docker build",
44 44
 		Long:  dockerBuilderLong,
45 45
 		Run: func(c *cobra.Command, args []string) {
46
-			cmd.RunDockerBuild()
46
+			err := cmd.RunDockerBuild(c.Out())
47
+			kcmdutil.CheckErr(err)
47 48
 		},
48 49
 	}
49 50
 	cmd.AddCommand(version.NewVersionCommand(name, false))
... ...
@@ -30,7 +30,7 @@ var _ = g.Describe("[builds][Conformance] build without output image", func() {
30 30
 				fmt.Fprintln(g.GinkgoWriter, out)
31 31
 			}
32 32
 			o.Expect(err).NotTo(o.HaveOccurred())
33
-			o.Expect(out).Should(o.ContainSubstring(`Build does not have an Output defined, no output image was pushed to a registry.`))
33
+			o.Expect(out).Should(o.ContainSubstring(`Build complete, no image push requested`))
34 34
 		})
35 35
 
36 36
 		g.It(fmt.Sprintf("should create an image from %q S2i template without an output image reference defined", s2iImageFixture), func() {
... ...
@@ -43,7 +43,7 @@ var _ = g.Describe("[builds][Conformance] build without output image", func() {
43 43
 				fmt.Fprintln(g.GinkgoWriter, out)
44 44
 			}
45 45
 			o.Expect(err).NotTo(o.HaveOccurred())
46
-			o.Expect(out).Should(o.ContainSubstring(`Build does not have an Output defined, no output image was pushed to a registry.`))
46
+			o.Expect(out).Should(o.ContainSubstring(`Build complete, no image push requested`))
47 47
 		})
48 48
 	})
49 49
 })