This commit added the following things in the code:

1. If action timeout is not provided in template, tink-worker should wait for that action to complete as long as it takes
2. Added few test cases around template validation

Signed-off-by: parauliya <[email protected]>

cmd/tink-worker/action.go

@@ -43,15 +43,13 @@ func executeAction(ctx context.Context, action *pb.WorkflowAction, wfID string)
 		return pb.ActionState_ACTION_IN_PROGRESS, errors.Wrap(err, "DOCKER CREATE")
 	}
 	l.With("containerID", id, "command", action.GetOnTimeout()).Info("container created")
-
-	var timeCtx context.Context
-	var cancel context.CancelFunc
+	// Setting time context for action
+	timeCtx := ctx
 	if action.Timeout > 0 {
-		timeCtx, cancel = context.WithTimeout(context.Background(), time.Duration(action.Timeout)*time.Second)
-	} else {
-		timeCtx, cancel = context.WithTimeout(context.Background(), 1*time.Hour)
+		var cancel context.CancelFunc
+		timeCtx, cancel = context.WithTimeout(ctx, time.Duration(action.Timeout)*time.Second)
+		defer cancel()
 	}
-	defer cancel()
 	err = startContainer(timeCtx, l, id)
 	if err != nil {
 		return pb.ActionState_ACTION_IN_PROGRESS, errors.Wrap(err, "DOCKER RUN")

db/mock/mock.go

@@ -22,7 +22,7 @@ type DB struct {
 	GetWorkflowActionsFunc           func(ctx context.Context, wfID string) (*pb.WorkflowActionList, error)
 	UpdateWorkflowStateFunc          func(ctx context.Context, wfContext *pb.WorkflowContext) error
 	InsertIntoWorkflowEventTableFunc func(ctx context.Context, wfEvent *pb.WorkflowActionStatus, time time.Time) error
-
 	// template
+	TemplateDB      map[string]interface{}
 	GetTemplateFunc func(ctx context.Context, id string) (string, string, error)
 }

db/mock/template.go

@@ -8,30 +8,26 @@ import (
 	"github.com/google/uuid"
 )
 
-type template struct {
-	id   uuid.UUID
-	data string
+type Template struct {
+	ID   uuid.UUID
+	Data string
 }
 
-var templateDB = map[string]interface{}{}
-
 // CreateTemplate creates a new workflow template
 func (d DB) CreateTemplate(ctx context.Context, name string, data string, id uuid.UUID) error {
-	if len(templateDB) > 0 {
-		if _, ok := templateDB[name]; ok {
-			return errors.New("Template name already exist in the database")
-		}
-		templateDB[name] = template{
-			id:   id,
-			data: data,
-		}
+	if d.TemplateDB == nil {
+		d.TemplateDB = make(map[string]interface{})
+	}
+
+	if _, ok := d.TemplateDB[name]; ok {
+		return errors.New("Template name already exist in the database")
+	}
 
-	} else {
-		templateDB[name] = template{
-			id:   id,
-			data: data,
-		}
+	d.TemplateDB[name] = Template{
+		ID:   id,
+		Data: data,
 	}
+
 	return nil
 }
 
@@ -42,12 +38,10 @@ func (d DB) GetTemplate(ctx context.Context, id string) (string, string, error)
 
 // DeleteTemplate deletes a workflow template
 func (d DB) DeleteTemplate(ctx context.Context, name string) error {
-	if len(templateDB) > 0 {
-		if _, ok := templateDB[name]; !ok {
-			return errors.New("Template name does not exist")
-		}
-		delete(templateDB, name)
+	if d.TemplateDB != nil {
+		delete(d.TemplateDB, name)
 	}
+
 	return nil
 }
 
@@ -63,7 +57,5 @@ func (d DB) UpdateTemplate(ctx context.Context, name string, data string, id uui
 
 // ClearTemplateDB clear all the templates
 func (d DB) ClearTemplateDB() {
-	for name := range templateDB {
-		delete(templateDB, name)
-	}
+	d.TemplateDB = make(map[string]interface{})
 }

db/template.go

@@ -9,6 +9,7 @@ import (
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
+	"github.com/tinkerbell/tink/pkg"
 )
 
 // CreateTemplate creates a new workflow template
@@ -18,6 +19,15 @@ func (d TinkDB) CreateTemplate(ctx context.Context, name string, data string, id
 		return errors.Wrap(err, "BEGIN transaction")
 	}
 
+	wf, err := pkg.ParseYAML([]byte(data))
+	if err != nil {
+		return err
+	}
+	err = pkg.ValidateTemplate(wf)
+	if err != nil {
+		return err
+	}
+
 	_, err = tx.Exec(`
 	INSERT INTO
 		template (created_at, updated_at, name, data, id)

go.mod

@@ -32,6 +32,7 @@ require (
 	github.com/spf13/viper v1.4.0
 	github.com/stretchr/testify v1.3.0
 	go.mongodb.org/mongo-driver v1.1.2 // indirect
+	golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2
 	golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e // indirect
 	golang.org/x/sys v0.0.0-20200331124033-c3d80250170d // indirect
 	golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect

grpc-server/template_test.go

@@ -22,23 +22,23 @@ tasks:
       timeout: 60`
 
 	template2 = `version: "0.1"
-name: hello_world_workflow
+name: hello_world_again_workflow
 global_timeout: 600
 tasks:
   - name: "hello world again"
-    worker: "{{.device_1}}"
+    worker: "{{.device_2}}"
     actions:
-  	- name: "hello_world_again"
+    - name: "hello_world_again"
       image: hello-world
       timeout: 60`
 )
 
 func TestCreateTemplate(t *testing.T) {
 	type (
 		args struct {
-			db        mock.DB
-			name      []string
-			templates []string
+			db       mock.DB
+			name     string
+			template string
 		}
 		want struct {
 			expectedError bool
@@ -50,9 +50,11 @@ func TestCreateTemplate(t *testing.T) {
 	}{
 		"SuccessfullTemplateCreation": {
 			args: args{
-				db:        mock.DB{},
-				name:      []string{"template_1"},
-				templates: []string{template1},
+				db: mock.DB{
+					TemplateDB: make(map[string]interface{}),
+				},
+				name:     "template_1",
+				template: template1,
 			},
 			want: want{
 				expectedError: false,
@@ -61,9 +63,13 @@ func TestCreateTemplate(t *testing.T) {
 
 		"SuccessfullMultipleTemplateCreation": {
 			args: args{
-				db:        mock.DB{},
-				name:      []string{"template_1", "template_2"},
-				templates: []string{template1, template2},
+				db: mock.DB{
+					TemplateDB: map[string]interface{}{
+						"template_1": template1,
+					},
+				},
+				name:     "template_2",
+				template: template2,
 			},
 			want: want{
 				expectedError: false,
@@ -72,35 +78,31 @@ func TestCreateTemplate(t *testing.T) {
 
 		"FailedMultipleTemplateCreationWithSameName": {
 			args: args{
-				db:        mock.DB{},
-				name:      []string{"template_1", "template_1"},
-				templates: []string{template1, template2},
+				db: mock.DB{
+					TemplateDB: map[string]interface{}{
+						"template_1": template1,
+					},
+				},
+				name:     "template_1",
+				template: template2,
 			},
 			want: want{
 				expectedError: true,
 			},
 		},
 	}
-	for name, tc := range testCases {
+
+	for name := range testCases {
+		tc := testCases[name]
 		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 			s := testServer(tc.args.db)
-			tc.args.db.ClearTemplateDB()
-			index := 0
-			res, err := s.CreateTemplate(context.TODO(), &pb.WorkflowTemplate{Name: tc.args.name[index], Data: tc.args.templates[index]})
-			assert.Nil(t, err)
-			assert.NotNil(t, res)
-			if err == nil && len(tc.args.templates) > 1 {
-				index++
-				_, err = s.CreateTemplate(context.TODO(), &pb.WorkflowTemplate{Name: tc.args.name[index], Data: tc.args.templates[index]})
-			} else {
-				return
-			}
-			if err != nil {
+			res, err := s.CreateTemplate(context.TODO(), &pb.WorkflowTemplate{Name: tc.args.name, Data: tc.args.template})
+			if tc.want.expectedError {
 				assert.Error(t, err)
-				assert.True(t, tc.want.expectedError)
 			} else {
 				assert.Nil(t, err)
-				assert.False(t, tc.want.expectedError)
+				assert.NotEmpty(t, res)
 			}
 		})
 	}

pkg/template_validator.go

@@ -1,18 +1,17 @@
 package pkg
 
 import (
-	"fmt"
-
 	"github.com/docker/distribution/reference"
+	"github.com/pkg/errors"
 	"gopkg.in/yaml.v2"
 )
 
 const (
-	errEmptyName           = "task/action name cannot be empty: %v"
-	errInvalidLength       = "task/action name cannot have more than 200 characters: %v"
-	errDuplicateTaskName   = "two tasks in a template cannot have same name: %v"
-	errInvalidActionImage  = "invalid action image: %v"
-	errDuplicateActionName = "two actions in a task cannot have same name: %v"
+	errEmptyName           = "task/action name cannot be empty: "
+	errInvalidLength       = "task/action name cannot have more than 200 characters: "
+	errDuplicateTaskName   = "two tasks in a template cannot have same name: "
+	errInvalidActionImage  = "invalid action image: "
+	errDuplicateActionName = "two actions in a task cannot have same name: "
 )
 
 // ParseYAML parses the template yaml content
@@ -30,50 +29,49 @@ func ParseYAML(yamlContent []byte) (*Workflow, error) {
 func ValidateTemplate(wf *Workflow) error {
 	taskNameMap := make(map[string]struct{})
 	for _, task := range wf.Tasks {
-		err := hasValidLength(task.Name)
-		if err != nil {
-			return err
+		if hasEmptyName(task.Name) {
+			return errors.New(errEmptyName + task.Name)
+		}
+		if !hasValidLength(task.Name) {
+			return errors.New(errInvalidLength + task.Name)
 		}
 		_, ok := taskNameMap[task.Name]
 		if ok {
-			return fmt.Errorf(errDuplicateTaskName, task.Name)
+			return errors.New(errDuplicateTaskName + task.Name)
 		}
 		taskNameMap[task.Name] = struct{}{}
 		actionNameMap := make(map[string]struct{})
 		for _, action := range task.Actions {
-			err := hasValidLength(action.Name)
-			if err != nil {
-				return err
+			if hasEmptyName(action.Name) {
+				return errors.New(errEmptyName + action.Name)
+			}
+
+			if !hasValidLength(action.Name) {
+				return errors.New(errInvalidLength + action.Name)
 			}
-			err = isValidImageName(action.Image)
-			if err != nil {
-				return fmt.Errorf(errInvalidActionImage, action.Image)
+
+			if !hasValidImageName(action.Image) {
+				return errors.New(errInvalidActionImage + action.Image)
 			}
 
 			_, ok := actionNameMap[action.Name]
 			if ok {
-				return fmt.Errorf(errDuplicateActionName, action.Name)
+				return errors.New(errDuplicateActionName + action.Name)
 			}
 			actionNameMap[action.Name] = struct{}{}
 		}
 	}
 	return nil
 }
 
-func hasValidLength(name string) error {
-	if name == "" {
-		return fmt.Errorf(errEmptyName, name)
-	}
-	if len(name) > 200 {
-		return fmt.Errorf(errInvalidLength, name)
-	}
-	return nil
+func hasEmptyName(name string) bool {
+	return name == ""
+}
+func hasValidLength(name string) bool {
+	return len(name) < 200
 }
 
-func isValidImageName(name string) error {
+func hasValidImageName(name string) bool {
 	_, err := reference.ParseNormalizedNamed(name)
-	if err != nil {
-		return err
-	}
-	return nil
+	return err == nil
 }

pkg/template_validator_test.go

@@ -97,6 +97,11 @@ func TestValidateTemplate(t *testing.T) {
 			wf:            workflow(withDuplicateActionName()),
 			expectedError: true,
 		},
+		{
+			name:          "long action name",
+			wf:            workflow(withLongActionName()),
+			expectedError: true,
+		},
 		{
 			name: "valid task name",
 			wf:   workflow(),
@@ -122,6 +127,12 @@ func withLongTaskName() workflowModifier {
 	}
 }
 
+func withLongActionName() workflowModifier {
+	return func(wf *Workflow) {
+		wf.Tasks[0].Actions[0].Name = "this action has a very long name to test whether we recevice an error or not if an action has very long name, one that would probably go beyond the limit of not having an action name with more than two hundred characters"
+	}
+}
+
 func withInvalidTaskName() workflowModifier {
 	return func(wf *Workflow) { wf.Tasks[0].Name = "" }
 }