Icinga DB Migration Script: Inform user about wrong/missing YAML entries

[LordHepipud]

I just ran into a problem on which the migration script for IDO to Icinga DB threw the following error:

2023-03-15T11:28:19.695+0100    FATAL   icingadb-migrate/main.go:449    Error 1048: Column 'environment_id' cannot be null
can't perform "INSERT INTO \"comment_history\" (\"entry_time\", \"author\", \"is_persistent\", \"expire_time\", \"removed_by\", \"entry_type\", \"comment_id\", \"service_id\", \"has_been_removed\", \"is_sticky\", \"environment_id\", \"endpoint_id\", \"object_type\", \"remove_time\", \"comment\", \"host_id\") VALUES (:entry_time, :author, :is_persistent, :expire_time, :removed_by, :entry_type, :comment_id, :service_id, :has_been_removed, :is_sticky, :environment_id, :endpoint_id, :object_type, :remove_time, :comment, :host_id) ON DUPLICATE KEY UPDATE \"entry_time\" = \"entry_time\""

The reason for that was the missing environment Id, which was set inside the YAML. However, two issues occurred on my side (full user error on my side):

• the section element was named Icinga2 instead of icinga2
• The env entry was added without indentation of two spaces

It would be great if the script would check the YAML before the execution and validate that the section icinga2 (or any other section mandatory) is present as well as the env entry is set on the correct indentation.

This would help a lot with troubleshooting.

[Al2Klimov]

TODO

• Config parsing: fail on unknown fields and print them #605
• Also scan maps for unknown keys as per Config parsing: fail on unknown fields and print them #605 (comment)
• Validate known keys

[julianbrost]

Validate known keys

Do you mean calling .Validate() on config types that support this? For the regular daemon config, there's already the following, but from a quick look, there seems to be no equivalent for the migration tool config:

icingadb/pkg/config/config.go

Lines 22 to 23 in a57f0cb

	// Validate checks constraints in the supplied configuration and returns an error if they are violated.
	func (c *Config) Validate() error {

So is that one an item just for the migration tool while the rest affects both that one and the daemon?

[Al2Klimov]

No, I thought of hooking our validators into https://pkg.go.dev/github.com/go-playground/validator/v10#Validate.Struct

[julianbrost]

What would this do better than the existing Validate() functions?

[Al2Klimov]

E.g.

diff --git a/pkg/config/config.go b/pkg/config/config.go
index 1388ba93..ca1e5cd5 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -4,6 +4,7 @@ import (
        "crypto/tls"
        "crypto/x509"
        "github.com/creasty/defaults"
+       "github.com/go-playground/validator/v10"
        "github.com/goccy/go-yaml"
        "github.com/jessevdk/go-flags"
        "github.com/pkg/errors"
@@ -54,7 +55,7 @@ func FromYAMLFile(name string) (*Config, error) {
        defer f.Close()

        c := &Config{}
-       d := yaml.NewDecoder(f)
+       d := yaml.NewDecoder(f, yaml.Validator(validator.New()))

        if err := defaults.Set(c); err != nil {
                return nil, errors.Wrap(err, "can't set config defaults")
diff --git a/pkg/config/database.go b/pkg/config/database.go
index b42ff8e1..560fe2e6 100644
--- a/pkg/config/database.go
+++ b/pkg/config/database.go
@@ -22,7 +22,7 @@ var registerDriverOnce sync.Once

 // Database defines database client configuration.
 type Database struct {
-       Type       string           `yaml:"type" default:"mysql"`
+       Type       string           `yaml:"type" default:"mysql" validate:"eq=mysql|eq=pgsql"`
        Host       string           `yaml:"host"`
        Port       int              `yaml:"port"`
        Database   string           `yaml:"database"`

would replace

invalid configuration: unknown database type "sql", must be one of: "mysql", "pgsql"

with

can't parse YAML file bad.yml: [2:9] Key: 'Database.Type' Error:Field validation for 'Type' failed on the 'eq=mysql|eq=pgsql' tag
   1 | database:
>  2 |   type: sql
               ^
   4 |   host: localhost
   5 |

Just the location display is awesome and the message is probably customisable.

[julianbrost]

I didn't know that this was integrated in the YAML parser. So that actually looks useful.

[Al2Klimov]

• Also scan maps for unknown keys as per Config parsing: fail on unknown fields and print them #605 (comment)

What about instead writing the possible options as struct fields? So they're noted somewhere AND re-use #605. Mappifying that struct automatically should be easy.

IMAO we could also let it as is. This point is really not super critical.

• Validate known keys

The errors have one hardcoded pattern:

https://github.com/go-playground/validator/blob/bd1113d5c1901c5205fc0a7af031a11268ff13ee/errors.go#L12-L14

So, nope:

the message is probably customisable.

I'll have to create and register tags with speaking names like greater_zero, so the error at least:

- validation for '%s' failed on the 'gte' tag
+ validation for '%s' failed on the 'greater_zero' tag

Or I'll register an additional tag making a note somewhere my wrapper of this validator will pick up.

[Al2Klimov]

There's an even better solution. How do you like

can't parse YAML file bad.yml: [2:9] must be one of: mysql, pgsql

   1 | database:
>  2 |   type: sql
               ^
   4 |   host: localhost
   5 |

(especially for migration which has two databases) at the cost of the following?

-       Type       string           `yaml:"type" default:"mysql"`
+       Type       string           `yaml:"type" default:"mysql" validate:"eq=mysql|eq=pgsql" errmsg:"must be one of: mysql, pgsql"`

-       if err := yaml.NewDecoder(cf).Decode(c); err != nil {
+       if err := yaml.NewDecoder(cf, Validator{}).Decode(c); err != nil {

package config

import (
	"github.com/go-playground/validator/v10"
	"github.com/goccy/go-yaml"
	"reflect"
	"strings"
)

var upstream = validator.New()

type Validator struct{}

func (Validator) Struct(s any) error {
	err := upstream.Struct(s)

	if err != nil {
		if ve, ok := err.(validator.ValidationErrors); ok {
			t := reflect.TypeOf(s)
			if t.Kind() == reflect.Ptr {
				t = t.Elem()
			}

			if t.Kind() == reflect.Struct {
				overridden := make(validationErrors, 0, len(ve))
				changed := false

				for _, e := range ve {
					if f, ok := t.FieldByName(e.StructField()); ok {
						if errmsg := f.Tag.Get("errmsg"); errmsg != "" {
							e = &fieldError{e, errmsg}
							changed = true
						}
					}

					overridden = append(overridden, e)
				}

				if changed {
					return overridden
				}
			}
		}
	}

	return err
}

type validationErrors []validator.FieldError

func (ve validationErrors) Error() string {
	var sb strings.Builder
	for _, e := range ve {
		sb.WriteString(e.Error())
		sb.WriteByte('\n')
	}

	return sb.String()
}

type fieldError struct {
	validator.FieldError

	err string
}

func (fe *fieldError) Error() string {
	return fe.err
}

Validator#Struct() intercepts the validator errors. For each of them, if the struct field exists and provides an errmsg, it uses the latter instead of the generic one. That's it, despite the complex look.

Of course to override just the error message, I need fieldError.

Finally, validator.ValidationErrors are picky about what they contain (can't parse YAML file bad.yml: [2:9] %!s(PANIC=Error method: interface conversion: validator.FieldError is *config.fieldError, not *validator.fieldError)), that's the story behind validationErrors.