[ws-manager] Improve workspace class name validation

components/common-go/testing/fixtures.go

@@ -113,6 +113,7 @@ func (ft *FixtureTest) Run() {
 				t.Errorf("cannot read golden file %s: %v", goldenFilePath, err)
 				return
 			}
+			expected = bytes.TrimSpace(expected)
 
 			if !bytes.Equal(actual, expected) {
 				expectedResult := ft.Gold()
@@ -128,8 +129,10 @@ func (ft *FixtureTest) Run() {
 				}
 
 				diff := deep.Equal(expectedResult, result)
+				if len(diff) > 0 {
+					t.Errorf("fixture %s: %v", fn, diff)
+				}
 
-				t.Errorf("fixture %s: %v", fn, diff)
 				return
 			}
 		})

components/ws-manager-api/go/config/config.go

@@ -12,18 +12,22 @@ import (
 	"os"
 	"path/filepath"
 
-	validation "github.com/go-ozzo/ozzo-validation"
+	ozzo "github.com/go-ozzo/ozzo-validation"
 	"github.com/go-ozzo/ozzo-validation/is"
 	"golang.org/x/xerrors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/yaml"
 
 	"github.com/gitpod-io/gitpod/common-go/grpc"
 	"github.com/gitpod-io/gitpod/common-go/util"
 	cntntcfg "github.com/gitpod-io/gitpod/content-service/api/config"
 )
 
+// DefaultWorkspaceClass is the name of the default workspace class
+const DefaultWorkspaceClass = "default"
+
 type osFS struct{}
 
 func (*osFS) Open(name string) (iofs.File, error) {
@@ -184,15 +188,15 @@ type WorkspaceDaemonConfiguration struct {
 
 // Validate validates the configuration to catch issues during startup and not at runtime
 func (c *Configuration) Validate() error {
-	err := validation.ValidateStruct(&c.Timeouts,
-		validation.Field(&c.Timeouts.AfterClose, validation.Required),
-		validation.Field(&c.Timeouts.HeadlessWorkspace, validation.Required),
-		validation.Field(&c.Timeouts.Initialization, validation.Required),
-		validation.Field(&c.Timeouts.RegularWorkspace, validation.Required),
-		validation.Field(&c.Timeouts.MaxLifetime, validation.Required),
-		validation.Field(&c.Timeouts.TotalStartup, validation.Required),
-		validation.Field(&c.Timeouts.ContentFinalization, validation.Required),
-		validation.Field(&c.Timeouts.Stopping, validation.Required),
+	err := ozzo.ValidateStruct(&c.Timeouts,
+		ozzo.Field(&c.Timeouts.AfterClose, ozzo.Required),
+		ozzo.Field(&c.Timeouts.HeadlessWorkspace, ozzo.Required),
+		ozzo.Field(&c.Timeouts.Initialization, ozzo.Required),
+		ozzo.Field(&c.Timeouts.RegularWorkspace, ozzo.Required),
+		ozzo.Field(&c.Timeouts.MaxLifetime, ozzo.Required),
+		ozzo.Field(&c.Timeouts.TotalStartup, ozzo.Required),
+		ozzo.Field(&c.Timeouts.ContentFinalization, ozzo.Required),
+		ozzo.Field(&c.Timeouts.Stopping, ozzo.Required),
 	)
 	if err != nil {
 		return xerrors.Errorf("timeouts: %w", err)
@@ -201,27 +205,33 @@ func (c *Configuration) Validate() error {
 		return xerrors.Errorf("stopping timeout must be greater than content finalization timeout")
 	}
 
-	err = validation.ValidateStruct(c,
-		validation.Field(&c.WorkspaceURLTemplate, validation.Required, validWorkspaceURLTemplate),
-		validation.Field(&c.WorkspaceHostPath, validation.Required),
-		validation.Field(&c.HeartbeatInterval, validation.Required),
-		validation.Field(&c.GitpodHostURL, validation.Required, is.URL),
-		validation.Field(&c.ReconnectionInterval, validation.Required),
+	err = ozzo.ValidateStruct(c,
+		ozzo.Field(&c.WorkspaceURLTemplate, ozzo.Required, validWorkspaceURLTemplate),
+		ozzo.Field(&c.WorkspaceHostPath, ozzo.Required),
+		ozzo.Field(&c.HeartbeatInterval, ozzo.Required),
+		ozzo.Field(&c.GitpodHostURL, ozzo.Required, is.URL),
+		ozzo.Field(&c.ReconnectionInterval, ozzo.Required),
 	)
 	if err != nil {
 		return err
 	}
 
+	if _, ok := c.WorkspaceClasses[DefaultWorkspaceClass]; !ok {
+		return xerrors.Errorf("missing \"%s\" workspace class", DefaultWorkspaceClass)
+	}
 	for name, class := range c.WorkspaceClasses {
+		if errs := validation.IsValidLabelValue(name); len(errs) > 0 {
+			return xerrors.Errorf("workspace class name \"%s\" is invalid: %v", name, errs)
+		}
 		if err := class.Container.Validate(); err != nil {
 			return xerrors.Errorf("workspace class %s: %w", name, err)
 		}
 
-		err = validation.ValidateStruct(&class.Templates,
-			validation.Field(&class.Templates.DefaultPath, validPodTemplate),
-			validation.Field(&class.Templates.PrebuildPath, validPodTemplate),
-			validation.Field(&class.Templates.ProbePath, validPodTemplate),
-			validation.Field(&class.Templates.RegularPath, validPodTemplate),
+		err = ozzo.ValidateStruct(&class.Templates,
+			ozzo.Field(&class.Templates.DefaultPath, validPodTemplate),
+			ozzo.Field(&class.Templates.PrebuildPath, validPodTemplate),
+			ozzo.Field(&class.Templates.ProbePath, validPodTemplate),
+			ozzo.Field(&class.Templates.RegularPath, validPodTemplate),
 		)
 		if err != nil {
 			return xerrors.Errorf("workspace class %s: %w", name, err)
@@ -231,7 +241,7 @@ func (c *Configuration) Validate() error {
 	return err
 }
 
-var validPodTemplate = validation.By(func(o interface{}) error {
+var validPodTemplate = ozzo.By(func(o interface{}) error {
 	s, ok := o.(string)
 	if !ok {
 		return xerrors.Errorf("field should be string")
@@ -241,7 +251,7 @@ var validPodTemplate = validation.By(func(o interface{}) error {
 	return err
 })
 
-var validWorkspaceURLTemplate = validation.By(func(o interface{}) error {
+var validWorkspaceURLTemplate = ozzo.By(func(o interface{}) error {
 	s, ok := o.(string)
 	if !ok {
 		return xerrors.Errorf("field should be string")
@@ -268,10 +278,10 @@ type PVCConfiguration struct {
 
 // Validate validates a PVC configuration
 func (c *PVCConfiguration) Validate() error {
-	return validation.ValidateStruct(c,
-		validation.Field(&c.Size, validation.Required),
-		validation.Field(&c.StorageClass, validation.Required),
-		validation.Field(&c.SnapshotClass, validation.Required),
+	return ozzo.ValidateStruct(c,
+		ozzo.Field(&c.Size, ozzo.Required),
+		ozzo.Field(&c.StorageClass, ozzo.Required),
+		ozzo.Field(&c.SnapshotClass, ozzo.Required),
 	)
 }
 
@@ -283,17 +293,20 @@ type ContainerConfiguration struct {
 
 // Validate validates a container configuration
 func (c *ContainerConfiguration) Validate() error {
-	return validation.ValidateStruct(c,
-		validation.Field(&c.Requests, validResourceConfig),
-		validation.Field(&c.Limits, validResourceConfig),
+	return ozzo.ValidateStruct(c,
+		ozzo.Field(&c.Requests, validResourceConfig),
+		ozzo.Field(&c.Limits, validResourceConfig),
 	)
 }
 
-var validResourceConfig = validation.By(func(o interface{}) error {
+var validResourceConfig = ozzo.By(func(o interface{}) error {
 	rc, ok := o.(*ResourceConfiguration)
 	if !ok {
 		return xerrors.Errorf("can only validate ResourceConfiguration")
 	}
+	if rc == nil {
+		return nil
+	}
 	if rc.CPU != "" {
 		_, err := resource.ParseQuantity(rc.CPU)
 		if err != nil {

components/ws-manager-api/go/config/config_test.go

@@ -6,6 +6,9 @@ package config
 
 import (
 	"testing"
+	"time"
+
+	"github.com/gitpod-io/gitpod/common-go/util"
 )
 
 func BenchmarkRenderWorkspacePortURL(b *testing.B) {
@@ -15,3 +18,66 @@ func BenchmarkRenderWorkspacePortURL(b *testing.B) {
 		RenderWorkspaceURL("{{.Port}}-{{.Prefix}}.{{.Host}}", "foo", "bar", "gitpod.io")
 	}
 }
+
+func TestValidate(t *testing.T) {
+	fromValidConfig := func(mod func(*Configuration)) *Configuration {
+		res := &Configuration{
+			Timeouts: WorkspaceTimeoutConfiguration{
+				TotalStartup:        util.Duration(10 * time.Second),
+				Initialization:      util.Duration(10 * time.Second),
+				RegularWorkspace:    util.Duration(10 * time.Second),
+				MaxLifetime:         util.Duration(10 * time.Second),
+				HeadlessWorkspace:   util.Duration(10 * time.Second),
+				AfterClose:          util.Duration(10 * time.Second),
+				ContentFinalization: util.Duration(10 * time.Second),
+				Stopping:            util.Duration(10 * time.Second),
+				Interrupted:         util.Duration(10 * time.Second),
+			},
+			WorkspaceClasses: map[string]*WorkspaceClass{
+				DefaultWorkspaceClass: {},
+			},
+			HeartbeatInterval:    util.Duration(10 * time.Second),
+			GitpodHostURL:        "https://gitpod.io",
+			ReconnectionInterval: util.Duration(10 * time.Second),
+			WorkspaceURLTemplate: "https://gitpod.io/foobar",
+			WorkspaceHostPath:    "/mnt/data",
+		}
+		mod(res)
+		return res
+	}
+
+	tests := []struct {
+		Name        string
+		Expectation string
+		Cfg         *Configuration
+	}{
+		{
+			Name: "missing default class",
+			Cfg: fromValidConfig(func(c *Configuration) {
+				delete(c.WorkspaceClasses, DefaultWorkspaceClass)
+			}),
+			Expectation: `missing "default" workspace class`,
+		},
+		{
+			Name: "invalid workspace class name",
+			Cfg: fromValidConfig(func(c *Configuration) {
+				c.WorkspaceClasses["not/a/valid/name"] = &WorkspaceClass{}
+			}),
+			Expectation: `workspace class name "not/a/valid/name" is invalid: [a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')]`,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			err := test.Cfg.Validate()
+
+			var errMsg string
+			if err != nil {
+				errMsg = err.Error()
+			}
+
+			if errMsg != test.Expectation {
+				t.Errorf("unexpected validation result: expect \"%s\", got \"%s\"", test.Expectation, errMsg)
+			}
+		})
+	}
+}

components/ws-manager/pkg/manager/create.go

@@ -19,6 +19,8 @@ import (
 	"github.com/imdario/mergo"
 	"github.com/opentracing/opentracing-go"
 	"golang.org/x/xerrors"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -846,6 +848,20 @@ func (m *Manager) newStartWorkspaceContext(ctx context.Context, req *api.StartWo
 	workspaceSpan := opentracing.StartSpan("workspace", opentracing.FollowsFrom(opentracing.SpanFromContext(ctx).Context()))
 	traceID := tracing.GetTraceID(workspaceSpan)
 
+	clsName := req.Spec.Class
+	if _, ok := m.Config.WorkspaceClasses[req.Spec.Class]; clsName == "" || !ok {
+		// For the time being, if the requested workspace class is unknown, or if
+		// no class is specified, we'll fall back to the default class.
+		clsName = config.DefaultWorkspaceClass
+	}
+
+	var class *config.WorkspaceClass
+	if cls, ok := m.Config.WorkspaceClasses[clsName]; ok {
+		class = cls
+	} else {
+		return nil, status.Errorf(codes.InvalidArgument, "workspace class \"%s\" is unknown", clsName)
+	}
+
 	labels := map[string]string{
 		"app":                  "gitpod",
 		"component":            "workspace",
@@ -855,17 +871,7 @@ func (m *Manager) newStartWorkspaceContext(ctx context.Context, req *api.StartWo
 		wsk8s.TypeLabel:        workspaceType,
 		headlessLabel:          fmt.Sprintf("%v", headless),
 		markerLabel:            "true",
-	}
-
-	var class *config.WorkspaceClass
-	if cls, ok := m.Config.WorkspaceClasses[req.Spec.Class]; ok {
-		class = cls
-		if req.Spec.Class != "" {
-			labels[workspaceClassLabel] = req.Spec.Class
-		}
-	} else {
-		// TODO(cw): in the future we should fail the request here. Until we've migrated server, let's not be that strict
-		// return nil, status.Errorf(codes.InvalidArgument, "workspace class \"%s\" is unknown", req.Spec.Class)
+		workspaceClassLabel:    clsName,
 	}
 
 	return &startWorkspaceContext{

components/ws-manager/pkg/manager/create_test.go

@@ -74,31 +74,22 @@ func TestCreateDefiniteWorkspacePod(t *testing.T) {
 				fixture.Classes = make(map[string]WorkspaceClass)
 			}
 
-			var (
-				files   []tpl
-				classes = make(map[string]*config.WorkspaceClass)
-			)
-			classes[""] = mgmtCfg.WorkspaceClasses[""]
-			fixture.Classes[""] = fixture.WorkspaceClass
-			if fixture.Classes[""].ResourceLimits == nil {
-				v := fixture.Classes[""]
-				v.ResourceLimits = mgmtCfg.WorkspaceClasses[""].Container.Limits
-				fixture.Classes[""] = v
-			}
-			if fixture.Classes[""].ResourceRequests == nil {
-				v := fixture.Classes[""]
-				v.ResourceRequests = mgmtCfg.WorkspaceClasses[""].Container.Requests
-				fixture.Classes[""] = v
+			var files []tpl
+			if _, exists := fixture.Classes[config.DefaultWorkspaceClass]; !exists {
+				if fixture.WorkspaceClass.ResourceLimits != nil || fixture.WorkspaceClass.ResourceRequests != nil {
+					// there's no default class in the fixture. If there are limits configured, use those
+					fixture.Classes[config.DefaultWorkspaceClass] = fixture.WorkspaceClass
+				}
 			}
+
 			for n, cls := range fixture.Classes {
 				var cfgCls config.WorkspaceClass
 				cfgCls.Container.Requests = cls.ResourceRequests
 				cfgCls.Container.Limits = cls.ResourceLimits
 
 				files = append(files, toTpl(n, cls, &cfgCls.Templates)...)
-				classes[n] = &cfgCls
+				mgmtCfg.WorkspaceClasses[n] = &cfgCls
 			}
-			mgmtCfg.WorkspaceClasses = classes
 
 			manager := &Manager{Config: mgmtCfg}
 

components/ws-manager/pkg/manager/testdata/cdwp_admission.golden

@@ -8,6 +8,7 @@
                 "app": "gitpod",
                 "component": "workspace",
                 "gitpod.io/networkpolicy": "default",
+                "gitpod.io/workspaceClass": "default",
                 "gpwsman": "true",
                 "headless": "false",
                 "metaID": "foobar",

components/ws-manager/pkg/manager/testdata/cdwp_affinity.golden

@@ -8,6 +8,7 @@
                 "app": "gitpod",
                 "component": "workspace",
                 "gitpod.io/networkpolicy": "default",
+                "gitpod.io/workspaceClass": "default",
                 "gpwsman": "true",
                 "headless": "false",
                 "metaID": "foobar",
@@ -222,10 +223,6 @@
                                     {
                                         "key": "gitpod.io/registry-facade_ready_ns_default",
                                         "operator": "Exists"
-                                    },
-                                    {
-                                        "key": "foobar",
-                                        "operator": "Exists"
                                     }
                                 ]
                             }