From ee9cb99875622d547085c490877fe3fd3b3c00dd Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 27 Mar 2024 16:43:51 -0700 Subject: [PATCH 1/9] feat: associate validation errors with the config section that caused them --- conf/config_validator.go | 83 ++++++++++++++++++++++- conf/config_validator_test.go | 123 ++++++++++++++++++++++++++++++++++ integration_test.go | 36 +++++----- 3 files changed, 223 insertions(+), 19 deletions(-) diff --git a/conf/config_validator.go b/conf/config_validator.go index 934e101..1d5189c 100644 --- a/conf/config_validator.go +++ b/conf/config_validator.go @@ -1,9 +1,11 @@ package conf import ( + "encoding/json" "errors" "fmt" "sort" + "strconv" "strings" "github.com/aerospike/aerospike-management-lib/asconfig" @@ -29,7 +31,7 @@ func NewConfigValidator(confHandler ConfHandler, mgmtLogger logr.Logger, version } // Validate validates the parsed configuration against the schema for the given versions. -// ValidationErrors is not nil if any errors during validation occur. +// ValidationErrors is not nil if any errors occur during validation. func (cv *ConfigValidator) Validate() (*ValidationErrors, error) { valid, tempVerrs, err := cv.IsValid(cv.mgmtLogger, cv.version) @@ -43,6 +45,30 @@ func (cv *ConfigValidator) Validate() (*ValidationErrors, error) { } if !valid || err != nil || len(verrs.Errors) > 0 { + + config := cv.ToMap() + + jsonConfigStr, err := json.Marshal(config) + if err != nil { + return nil, err + } + + jsonConfig := map[string]any{} + err = json.Unmarshal(jsonConfigStr, &jsonConfig) + + // check the context of each error and use that context to get the name + // of the field that is causing the error from the json config + for i, verr := range verrs.Errors { + context, _ := strings.CutPrefix(verr.Context, "(root).") + context, err := jsonToConfigContext(jsonConfig, context) + if err != nil { + fmt.Println(err) + continue + } + + verrs.Errors[i].Context = context + } + return &verrs, errors.Join(ErrConfigValidation, err) } @@ -107,3 +133,58 @@ func (o ValidationErrors) Error() string { return errString } + +func jsonToConfigContext(jsonConfig any, context string) (string, error) { + split := strings.SplitN(context, ".", 2) + key := split[0] + + var res string + + if index, err := strconv.Atoi(key); err == nil { + jsonSlice, ok := jsonConfig.([]any) + if !ok { + return "", fmt.Errorf("context is not a slice in json config at: %s", context) + } + + indexedMap, ok := jsonSlice[index].(map[string]any) + if !ok { + return "", fmt.Errorf("context is not a map in json config at: %s", context) + } + + name, ok := indexedMap["name"] + if !ok { + return "", fmt.Errorf("name not found in json config at: %s", context) + } + + if nameStr, ok := name.(string); !ok { + return "", fmt.Errorf("name is not a string in json config at: %v", context) + } else { + res = nameStr + } + + jsonConfig = indexedMap + } else { + jsonMap, ok := jsonConfig.(map[string]any) + if !ok { + return "", fmt.Errorf("context is not a map in json config at: %s", context) + } + + jsonConfig, ok = jsonMap[key] + if !ok { + return "", fmt.Errorf("context not found in json config at: %s", key) + } + + res = key + } + + if len(split) > 1 { + val, err := jsonToConfigContext(jsonConfig, split[1]) + if err != nil { + return "", err + } + + res = fmt.Sprintf("%s.%s", res, val) + } + + return res, nil +} diff --git a/conf/config_validator_test.go b/conf/config_validator_test.go index dc802a7..874e768 100644 --- a/conf/config_validator_test.go +++ b/conf/config_validator_test.go @@ -81,3 +81,126 @@ func Test_asconf_Validate(t *testing.T) { }) } } + +func Test_jsonToConfigContext(t *testing.T) { + type args struct { + jsonConfig any + context string + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "positive", + args: args{ + jsonConfig: map[string]any{ + "test0": map[string]any{ + "name": "nested0", + "test1": "test2", + }, + "test3": "test4", + "test5": []any{ + "test6", + map[string]any{ + "name": "nested1", + "test7": "test8", + "test9": "test10", + "test11": []any{ + "test12", + "test13", + map[string]any{ + "name": "nested2", + "test14": "test15", + }, + }, + }, + }, + }, + context: "test5.1.test11.2", + }, + want: "test5.nested1.test11.nested2", + }, + { + name: "negative context is not a slice", + args: args{ + jsonConfig: map[string]any{ + "test0": map[string]any{ + "name": "nested0", + "test1": "test2", + }, + "test3": "test4", + }, + context: "test0.1.test1", + }, + wantErr: true, + }, + { + name: "negative context is not a map", + args: args{ + jsonConfig: map[string]any{ + "test0": []any{ + "test1", + }, + }, + context: "test0.0", + }, + wantErr: true, + }, + { + name: "negative name not found", + args: args{ + jsonConfig: map[string]any{ + "test0": []any{ + map[string]any{ + "test1": "test2", + }, + }, + }, + context: "test0.0", + }, + wantErr: true, + }, + { + name: "negative name is not a string", + args: args{ + jsonConfig: map[string]any{ + "test0": []any{ + map[string]any{ + "name": 1, + "test1": "test2", + }, + }, + }, + context: "test0.0", + }, + wantErr: true, + }, + { + name: "negative context is not a map, non numeric key", + args: args{ + jsonConfig: map[string]any{ + "test0": []any{ + "test1", + }, + }, + context: "test0.test1", + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := jsonToConfigContext(tt.args.jsonConfig, tt.args.context) + if (err != nil) != tt.wantErr { + t.Errorf("jsonToConfigContext() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("jsonToConfigContext() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/integration_test.go b/integration_test.go index 5d5d387..8505be3 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1016,24 +1016,24 @@ var validateTests = []validateTest{ arguments: []string{"validate", "-a", "7.0.0", filepath.Join(extraTestPath, "server64", "server64.yaml")}, expectError: true, source: filepath.Join(extraTestPath, "server64", "server64.yaml"), - expectedResult: `context: (root).namespaces.0 - - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed -context: (root).namespaces.0.storage-engine - - description: devices is required, error-type: required -context: (root).namespaces.1 - - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed -context: (root).namespaces.1.index-type - - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed - - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed - - description: mounts-budget is required, error-type: required -context: (root).namespaces.1.sindex-type - - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed - - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed - - description: mounts-budget is required, error-type: required -context: (root).namespaces.1.storage-engine - - description: devices is required, error-type: required -context: (root).service - - description: cluster-name is required, error-type: required + expectedResult: `context: namespaces.ns1 + - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed +context: namespaces.ns1.index-type + - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed + - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed + - description: mounts-budget is required, error-type: required +context: namespaces.ns1.sindex-type + - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed + - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed + - description: mounts-budget is required, error-type: required +context: namespaces.ns1.storage-engine + - description: devices is required, error-type: required +context: namespaces.ns2 + - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed +context: namespaces.ns2.storage-engine + - description: devices is required, error-type: required +context: service + - description: cluster-name is required, error-type: required `, }, } From 0c4db9e1cd083d89a3034b1357aee2aaf36c1881 Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 27 Mar 2024 16:46:52 -0700 Subject: [PATCH 2/9] remove debug print --- conf/config_validator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/conf/config_validator.go b/conf/config_validator.go index 1d5189c..663fa53 100644 --- a/conf/config_validator.go +++ b/conf/config_validator.go @@ -62,7 +62,8 @@ func (cv *ConfigValidator) Validate() (*ValidationErrors, error) { context, _ := strings.CutPrefix(verr.Context, "(root).") context, err := jsonToConfigContext(jsonConfig, context) if err != nil { - fmt.Println(err) + // if we can't associate the error with its + // corresponding field, just use the current context continue } From 126bd71df6a3918fb9cd638f792d66122a8ef72a Mon Sep 17 00:00:00 2001 From: dylan Date: Wed, 27 Mar 2024 16:54:28 -0700 Subject: [PATCH 3/9] document jsonToConfigContext --- conf/config_validator.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/conf/config_validator.go b/conf/config_validator.go index 663fa53..ccb279e 100644 --- a/conf/config_validator.go +++ b/conf/config_validator.go @@ -135,50 +135,80 @@ func (o ValidationErrors) Error() string { return errString } +// jsonToConfigContext takes a json config and a context string and returns a copy +// of the context string with the field names instead of list indexes. +// For example, if the context is "namespaces.0", and the json config is: +// +// { +// "namespaces": [ +// { +// "name": "test" +// } +// ] +// } +// +// The function will return "namespaces.test" +// An error is returned if the json or context are not in the expected management lib format. func jsonToConfigContext(jsonConfig any, context string) (string, error) { split := strings.SplitN(context, ".", 2) key := split[0] var res string + // check if key is an index if index, err := strconv.Atoi(key); err == nil { + // if key is an index, then context should be a slice jsonSlice, ok := jsonConfig.([]any) if !ok { return "", fmt.Errorf("context is not a slice in json config at: %s", context) } + // check if index is out of bounds + if len(jsonSlice) <= index { + return "", fmt.Errorf("index out of bounds json config at: %s", context) + } + + // the indexed object should be a map indexedMap, ok := jsonSlice[index].(map[string]any) if !ok { return "", fmt.Errorf("context is not a map in json config at: %s", context) } + // get the name field from the indexed object name, ok := indexedMap["name"] if !ok { return "", fmt.Errorf("name not found in json config at: %s", context) } + // name should be a string if nameStr, ok := name.(string); !ok { return "", fmt.Errorf("name is not a string in json config at: %v", context) } else { + // set res to the name instead of the index res = nameStr } + // set jsonConfig to the indexed object jsonConfig = indexedMap } else { + // if key is not an index, then context should be a map jsonMap, ok := jsonConfig.(map[string]any) if !ok { return "", fmt.Errorf("context is not a map in json config at: %s", context) } + // set jsonConfig to the object at key jsonConfig, ok = jsonMap[key] if !ok { return "", fmt.Errorf("context not found in json config at: %s", key) } + // set res to the context key res = key } if len(split) > 1 { + // if we have more context to parse, recurse val, err := jsonToConfigContext(jsonConfig, split[1]) if err != nil { return "", err From ce258c2afd7ae829b73838d620836f76888c6adf Mon Sep 17 00:00:00 2001 From: dylan Date: Thu, 28 Mar 2024 13:00:48 -0700 Subject: [PATCH 4/9] chore: log which config file asconfig is using at the debug log level --- cmd/root.go | 2 +- integration_test.go | 24 +++++++++++------------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 115c8da..6330bf8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -44,7 +44,7 @@ func newRootCmd() *cobra.Command { } if cfgFile != "" { - logger.Infof("Using config file: %s", cfgFile) + logger.Debugf("Using config file: %s", cfgFile) } lvl, err := cmd.Flags().GetString("log-level") diff --git a/integration_test.go b/integration_test.go index 8505be3..7732f28 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1,5 +1,3 @@ -//go:build integration - package main import ( @@ -1017,23 +1015,23 @@ var validateTests = []validateTest{ expectError: true, source: filepath.Join(extraTestPath, "server64", "server64.yaml"), expectedResult: `context: namespaces.ns1 - - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed + - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed context: namespaces.ns1.index-type - - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed - - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed - - description: mounts-budget is required, error-type: required + - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed + - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed + - description: mounts-budget is required, error-type: required context: namespaces.ns1.sindex-type - - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed - - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed - - description: mounts-budget is required, error-type: required + - description: Additional property mounts-high-water-pct is not allowed, error-type: additional_property_not_allowed + - description: Additional property mounts-size-limit is not allowed, error-type: additional_property_not_allowed + - description: mounts-budget is required, error-type: required context: namespaces.ns1.storage-engine - - description: devices is required, error-type: required + - description: devices is required, error-type: required context: namespaces.ns2 - - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed + - description: Additional property memory-size is not allowed, error-type: additional_property_not_allowed context: namespaces.ns2.storage-engine - - description: devices is required, error-type: required + - description: devices is required, error-type: required context: service - - description: cluster-name is required, error-type: required + - description: cluster-name is required, error-type: required `, }, } From f157de3972754802a3a9775a02fa0bc5dcab2aff Mon Sep 17 00:00:00 2001 From: dylan Date: Thu, 28 Mar 2024 15:49:01 -0700 Subject: [PATCH 5/9] add back integration build tag --- integration_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integration_test.go b/integration_test.go index 7732f28..d0d6ce0 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1,3 +1,5 @@ +//go:build integration + package main import ( From c0dfad673275453def42c54a4411cf2e720f7fd7 Mon Sep 17 00:00:00 2001 From: dylan Date: Tue, 2 Apr 2024 11:38:06 -0700 Subject: [PATCH 6/9] log config file at info level, force validate output on stdout --- cmd/root.go | 2 +- cmd/validate.go | 2 +- integration_test.go | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 6330bf8..115c8da 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -44,7 +44,7 @@ func newRootCmd() *cobra.Command { } if cfgFile != "" { - logger.Debugf("Using config file: %s", cfgFile) + logger.Infof("Using config file: %s", cfgFile) } lvl, err := cmd.Flags().GetString("log-level") diff --git a/cmd/validate.go b/cmd/validate.go index 4324639..9ba38d0 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -94,7 +94,7 @@ func newValidateCmd() *cobra.Command { if verrs != nil { // force validation errors to be written to stdout // so they can more easily be grepd etc. - cmd.Print(verrs.Error()) + fmt.Print(verrs.Error()) return errors.Join(conf.ErrConfigValidation, ErrSilent) } if err != nil { diff --git a/integration_test.go b/integration_test.go index d0d6ce0..7f768c0 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1042,7 +1042,7 @@ func TestValidate(t *testing.T) { for _, tf := range validateTests { com := exec.Command(binPath+"/asconfig.test", tf.arguments...) com.Env = []string{"GOCOVERDIR=" + coveragePath} - out, err := com.CombinedOutput() + out, err := com.Output() if tf.expectError == (err == nil) { t.Errorf("\nTESTCASE: %+v\nERR: %+v\n", tf.arguments, err) } @@ -1050,7 +1050,6 @@ func TestValidate(t *testing.T) { if string(out) != tf.expectedResult { t.Errorf("\nTESTCASE: %+v\nACTUAL: %s\nEXPECTED: %s", tf.arguments, string(out), tf.expectedResult) } - } } @@ -1065,7 +1064,7 @@ func TestStdinValidate(t *testing.T) { com := exec.Command(binPath+"/asconfig.test", tf.arguments...) com.Env = []string{"GOCOVERDIR=" + coveragePath} com.Stdin = in - out, err := com.CombinedOutput() + out, err := com.Output() if tf.expectError == (err == nil) { t.Errorf("\nTESTCASE: %+v\nERR: %+v\n", tf.arguments, err) } From 02bed4cb94b1074b56e26cd95941a8e0c1f7260c Mon Sep 17 00:00:00 2001 From: dylan Date: Tue, 2 Apr 2024 12:34:12 -0700 Subject: [PATCH 7/9] revert tools-2885, print validation errors to stderr --- cmd/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/validate.go b/cmd/validate.go index 9ba38d0..4324639 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -94,7 +94,7 @@ func newValidateCmd() *cobra.Command { if verrs != nil { // force validation errors to be written to stdout // so they can more easily be grepd etc. - fmt.Print(verrs.Error()) + cmd.Print(verrs.Error()) return errors.Join(conf.ErrConfigValidation, ErrSilent) } if err != nil { From 3f175dd78a6351d8596bb31a6a6ebc0ddfa4c25d Mon Sep 17 00:00:00 2001 From: dylan Date: Tue, 2 Apr 2024 12:40:02 -0700 Subject: [PATCH 8/9] log which config file is used after log level is applied --- cmd/root.go | 8 ++++---- integration_test.go | 22 ++++++++++------------ 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 115c8da..aa5f984 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -43,10 +43,6 @@ func newRootCmd() *cobra.Command { multiErr = errors.Join(multiErr, err) } - if cfgFile != "" { - logger.Infof("Using config file: %s", cfgFile) - } - lvl, err := cmd.Flags().GetString("log-level") if err != nil { multiErr = fmt.Errorf("%w, %w", multiErr, err) @@ -59,6 +55,10 @@ func newRootCmd() *cobra.Command { logger.SetLevel(lvlCode) + if cfgFile != "" { + logger.Infof("Using config file: %s", cfgFile) + } + return multiErr }, } diff --git a/integration_test.go b/integration_test.go index 7f768c0..5de861d 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1,5 +1,3 @@ -//go:build integration - package main import ( @@ -73,11 +71,11 @@ func TestMain(m *testing.M) { panic(err) } - featKeyDir = os.Getenv("FEATKEY_DIR") - fmt.Println(featKeyDir) - if featKeyDir == "" { - panic("FEATKEY_DIR environement variable must an absolute path to a directory containing valid aerospike feature key files featuresv1.conf and featuresv2.conf of feature key format 1 and 2 respectively.") - } + // featKeyDir = os.Getenv("FEATKEY_DIR") + // fmt.Println(featKeyDir) + // if featKeyDir == "" { + // panic("FEATKEY_DIR environement variable must an absolute path to a directory containing valid aerospike feature key files featuresv1.conf and featuresv2.conf of feature key format 1 and 2 respectively.") + // } code := m.Run() @@ -1001,19 +999,19 @@ type validateTest struct { var validateTests = []validateTest{ { - arguments: []string{"validate", "-a", "6.2.0", filepath.Join(sourcePath, "pmem_cluster_cr.yaml")}, + arguments: []string{"validate", "-a", "6.2.0", "-l", "panic", filepath.Join(sourcePath, "pmem_cluster_cr.yaml")}, expectError: false, expectedResult: "", source: filepath.Join(sourcePath, "pmem_cluster_cr.yaml"), }, { - arguments: []string{"validate", filepath.Join(extraTestPath, "metadata", "metadata.conf")}, + arguments: []string{"validate", "-l", "panic", filepath.Join(extraTestPath, "metadata", "metadata.conf")}, expectError: false, expectedResult: "", source: filepath.Join(extraTestPath, "metadata", "metadata.conf"), }, { - arguments: []string{"validate", "-a", "7.0.0", filepath.Join(extraTestPath, "server64", "server64.yaml")}, + arguments: []string{"validate", "-a", "7.0.0", "-l", "panic", filepath.Join(extraTestPath, "server64", "server64.yaml")}, expectError: true, source: filepath.Join(extraTestPath, "server64", "server64.yaml"), expectedResult: `context: namespaces.ns1 @@ -1042,7 +1040,7 @@ func TestValidate(t *testing.T) { for _, tf := range validateTests { com := exec.Command(binPath+"/asconfig.test", tf.arguments...) com.Env = []string{"GOCOVERDIR=" + coveragePath} - out, err := com.Output() + out, err := com.CombinedOutput() if tf.expectError == (err == nil) { t.Errorf("\nTESTCASE: %+v\nERR: %+v\n", tf.arguments, err) } @@ -1064,7 +1062,7 @@ func TestStdinValidate(t *testing.T) { com := exec.Command(binPath+"/asconfig.test", tf.arguments...) com.Env = []string{"GOCOVERDIR=" + coveragePath} com.Stdin = in - out, err := com.Output() + out, err := com.CombinedOutput() if tf.expectError == (err == nil) { t.Errorf("\nTESTCASE: %+v\nERR: %+v\n", tf.arguments, err) } From 282fe367f7973ef3d6a2c083de7e8e9baca8d455 Mon Sep 17 00:00:00 2001 From: dylan Date: Tue, 2 Apr 2024 12:43:17 -0700 Subject: [PATCH 9/9] add back integration tag --- cmd/validate.go | 2 -- integration_test.go | 12 +++++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/validate.go b/cmd/validate.go index 4324639..9e3efae 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -92,8 +92,6 @@ func newValidateCmd() *cobra.Command { verrs, err := conf.NewConfigValidator(asconfig, mgmtLibLogger, version).Validate() if verrs != nil { - // force validation errors to be written to stdout - // so they can more easily be grepd etc. cmd.Print(verrs.Error()) return errors.Join(conf.ErrConfigValidation, ErrSilent) } diff --git a/integration_test.go b/integration_test.go index 5de861d..380867f 100644 --- a/integration_test.go +++ b/integration_test.go @@ -1,3 +1,5 @@ +//go:build integration + package main import ( @@ -71,11 +73,11 @@ func TestMain(m *testing.M) { panic(err) } - // featKeyDir = os.Getenv("FEATKEY_DIR") - // fmt.Println(featKeyDir) - // if featKeyDir == "" { - // panic("FEATKEY_DIR environement variable must an absolute path to a directory containing valid aerospike feature key files featuresv1.conf and featuresv2.conf of feature key format 1 and 2 respectively.") - // } + featKeyDir = os.Getenv("FEATKEY_DIR") + fmt.Println(featKeyDir) + if featKeyDir == "" { + panic("FEATKEY_DIR environement variable must an absolute path to a directory containing valid aerospike feature key files featuresv1.conf and featuresv2.conf of feature key format 1 and 2 respectively.") + } code := m.Run()