Skip to content

Commit

Permalink
feat: improve error logs when running sync/diff/validate
Browse files Browse the repository at this point in the history
Currently decK doesn't return very useful and descriptive
errors when configuration files are validated. This is
particularly painful when the configuration is distributed
in several files and the error doesn't specificy which
file contains the error.

Let's take for example the following files:

```
$ cat services.yaml
services:
- name: svc1
  host: mockbin.org
  foo: bar

consumers:
- username: good
- name: bad
```

If we run decK, it will return some obscure and incomplete errors:

```
$ deck sync -s services.yaml -s consumers.yaml
Error: reading file: validating file content: 1 errors occurred:
        services.0: Additional property foo is not allowed
```

This commit is going to improve the way decK handles
configuration validation error logging in order to provide
a more useful description of the problems.
When running decK with the new functionality, the following will
be returned:

```
$ ./deck sync -s services.yaml -s consumers.yaml
Error: 2 errors occurred:
	reading file services.yaml: validating file content: 1 errors occurred:
	validation error: object="bar", err=services.0: Additional property foo is not allowed

	reading file consumers.yaml: validating file content: 3 errors occurred:
	validation error: object={"name":"bad"}, err=consumers.1: Must validate at least one schema (anyOf)
	validation error: object={"name":"bad"}, err=consumers.1: username is required
	validation error: object="bad", err=consumers.1: Additional property name is not allowed
```
  • Loading branch information
GGabriele committed Jul 19, 2023
1 parent d19b622 commit 105df59
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 30 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ test-all: lint test

.PHONY: test
test:
go test -race ./...
go test -race -count=1 ./...

.PHONY: lint
lint:
Expand Down
46 changes: 25 additions & 21 deletions file/readfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,44 +20,48 @@ import (
// file, depending on the type of each item in filenames, merges the content of
// these files and renders a Content.
func getContent(filenames []string) (*Content, error) {
var allReaders []io.Reader
var workspaces []string
var res Content
var errs []error
for _, fileOrDir := range filenames {
readers, err := getReaders(fileOrDir)
if err != nil {
return nil, err
}
allReaders = append(allReaders, readers...)
}
var res Content
for _, r := range allReaders {
content, err := readContent(r)
if err != nil {
return nil, fmt.Errorf("reading file: %w", err)
}
if content.Workspace != "" {
workspaces = append(workspaces, content.Workspace)
}
err = mergo.Merge(&res, content, mergo.WithAppendSlice)
if err != nil {
return nil, fmt.Errorf("merging file contents: %w", err)

for filename, r := range readers {
content, err := readContent(r)
if err != nil {
errs = append(errs, fmt.Errorf("reading file %s: %w", filename, err))
continue
}
if content.Workspace != "" {
workspaces = append(workspaces, content.Workspace)
}
err = mergo.Merge(&res, content, mergo.WithAppendSlice)
if err != nil {
return nil, fmt.Errorf("merging file contents: %w", err)
}
}
}
if len(errs) > 0 {
return nil, utils.ErrArray{Errors: errs}
}
if err := validateWorkspaces(workspaces); err != nil {
return nil, err
}
return &res, nil
}

// getReaders returns back io.Readers representing all the YAML and JSON
// files in a directory. If fileOrDir is a single file, then it
// getReaders returns back a map of filename:io.Reader representing all the
// YAML and JSON files in a directory. If fileOrDir is a single file, then it
// returns back the reader for the file.
// If fileOrDir is equal to "-" string, then it returns back a io.Reader
// for the os.Stdin file descriptor.
func getReaders(fileOrDir string) ([]io.Reader, error) {
func getReaders(fileOrDir string) (map[string]io.Reader, error) {
// special case where `-` means stdin
if fileOrDir == "-" {
return []io.Reader{os.Stdin}, nil
return map[string]io.Reader{"STDIN": os.Stdin}, nil
}

finfo, err := os.Stat(fileOrDir)
Expand All @@ -75,13 +79,13 @@ func getReaders(fileOrDir string) ([]io.Reader, error) {
files = append(files, fileOrDir)
}

var res []io.Reader
res := make(map[string]io.Reader, len(files))
for _, file := range files {
f, err := os.Open(file)
if err != nil {
return nil, fmt.Errorf("opening file: %w", err)
}
res = append(res, bufio.NewReader(f))
res[file] = bufio.NewReader(f)
}
return res, nil
}
Expand Down
46 changes: 40 additions & 6 deletions file/readfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import (
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/kong/deck/utils"
"github.com/kong/go-kong/kong"
"github.com/stretchr/testify/require"
)

func Test_configFilesInDir(t *testing.T) {
Expand Down Expand Up @@ -66,15 +67,17 @@ func Test_getReaders(t *testing.T) {
tests := []struct {
name string
args args
want []io.Reader
want map[string]io.Reader
// length of returned array
wantLen int
wantErr bool
}{
{
name: "read from standard input",
args: args{"-"},
want: []io.Reader{os.Stdin},
name: "read from standard input",
args: args{"-"},
want: map[string]io.Reader{
"STDIN": os.Stdin,
},
wantLen: 1,
wantErr: false,
},
Expand Down Expand Up @@ -126,6 +129,29 @@ func Test_getReaders(t *testing.T) {
}
}

func sortSlices(x, y interface{}) bool {
var xName, yName string
switch xEntity := x.(type) {
case FService:
yEntity := y.(FService)
xName = *xEntity.Name
yName = *yEntity.Name
case FRoute:
yEntity := y.(FRoute)
xName = *xEntity.Name
yName = *yEntity.Name
case FConsumer:
yEntity := y.(FConsumer)
xName = *xEntity.Username
yName = *yEntity.Username
case FPlugin:
yEntity := y.(FPlugin)
xName = *xEntity.Name
yName = *yEntity.Name
}
return xName < yName
}

func Test_getContent(t *testing.T) {
type args struct {
filenames []string
Expand Down Expand Up @@ -556,7 +582,15 @@ kong.log.set_serialize_value("span_id", parse_traceid(ngx.ctx.KONG_SPANS[1].span
t.Errorf("getContent() error = %v, wantErr %v", err, tt.wantErr)
return
}
require.Equal(t, tt.want, got)

opt := []cmp.Option{
cmpopts.SortSlices(sortSlices),
cmpopts.SortSlices(func(a, b *string) bool { return *a < *b }),
cmpopts.EquateEmpty(),
}
if diff := cmp.Diff(got, tt.want, opt...); diff != "" {
t.Errorf(diff)
}
})
}
}
Expand Down
19 changes: 17 additions & 2 deletions file/validate.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
package file

import (
"encoding/json"
"errors"
"fmt"

"github.com/kong/deck/utils"
"github.com/xeipuuv/gojsonschema"
"sigs.k8s.io/yaml"
)

type ValidationError struct {
Object string `json:"object"`
Err error `json:"error"`
}

func (e *ValidationError) Error() string {
return fmt.Sprintf("validation error: object=%s, err=%v", e.Object, e.Err)
}

func validate(content []byte) error {
var c map[string]interface{}
err := yaml.Unmarshal(content, &c)
Expand All @@ -24,10 +35,14 @@ func validate(content []byte) error {
if result.Valid() {
return nil
}

var errs utils.ErrArray
for _, desc := range result.Errors() {
err := fmt.Errorf(desc.String())
errs.Errors = append(errs.Errors, err)
jsonString, err := json.Marshal(desc.Value())
if err != nil {
return err
}
errs.Errors = append(errs.Errors, &ValidationError{Object: string(jsonString), Err: errors.New(desc.String())})
}
return errs
}
Expand Down

0 comments on commit 105df59

Please sign in to comment.