conntrack: config validation

[ronensc]

This PR adds validation checks to the connection tracking config.

I created an internal error type with a list of booleans to verify in the unit tests that the config under test is invalid because of the right reason. I'm not sure if this is the most idiomatic way to do this in Go. @jotak @mariomac please let me know if you have a better idea.

[codecov-commenter]

Codecov Report

Merging #285 (9d25269) into main (fb310b7) will increase coverage by 0.48%.
The diff coverage is 84.90%.

@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
+ Coverage   66.89%   67.38%   +0.48%     
==========================================
  Files          73       73              
  Lines        4157     4271     +114     
==========================================
+ Hits         2781     2878      +97     
- Misses       1197     1209      +12     
- Partials      179      184       +5     

Flag	Coverage Δ
unittests	67.38% <84.90%> (+0.48%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/pipeline/extract/conntrack/conntrack.go	89.02% <57.14%> (-1.66%)	⬇️
pkg/api/conntrack.go	86.86% <86.86%> (ø)
pkg/pipeline/encode/encode_prom.go	75.13% <0.00%> (-0.85%)	⬇️
pkg/api/encode_prom.go	100.00% <0.00%> (ø)
pkg/api/transform_filter.go	100.00% <0.00%> (ø)
pkg/pipeline/transform/transform_filter.go	95.74% <0.00%> (+1.15%)	⬆️
pkg/pipeline/ingest/ingest_collector.go	49.62% <0.00%> (+1.50%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jotak]

/lgtm
For dealing with these errors, I don't know what is idiomatic, most of the time I don't try to be smart and just log an error message, and just check for a partial string within that message in tests. It's perhaps less smart (e.g. tests can break easily when just the error text i modified), but it's less ceremony in code, so .. take the tradeof that you want :) personally I'm fine with both approach.

Or there could be another approach, defining a different error for every case. It's even more ceremony in errors definition :) but not on the caller side, and also, it's more accurate testing (e.g. you test not only the error type, but also its parameters) e.g.:

type fieldGroupABOnlyOneIsSetError struct{}

func newFieldGroupABOnlyOneIsSetError() error {
	return fieldGroupABOnlyOneIsSetError{}
}
func (e fieldGroupABOnlyOneIsSetError) Error() string {
	return "only one of 'fieldGroupARef' and 'fieldGroupBRef' is set. They should both be set or both unset"
}

type splitABWithNoBidiError struct{ name string }

func newSplitABWithNoBidiError(name string) error {
	return splitABWithNoBidiError{name}
}
func (e splitABWithNoBidiError) Error() string {
	return fmt.Sprintf("output field %q has splitAB=true although bidirection is not enabled (fieldGroupARef is empty)", e.name)
}
// Etc.

And on callers side:

	if isGroupAEmpty != isGroupBEmpty { // XOR
		return newFieldGroupABOnlyOneIsSetError()
	}

	isBidi := !isGroupAEmpty
	for _, of := range ct.OutputFields {
		if of.SplitAB && !isBidi {
			return newSplitABWithNoBidiError(of.Name)
		}

And test is like:

		{
			"splitAB in non bidirectional configuration",
			ConnTrack{
				KeyDefinition: KeyDefinition{},
				OutputFields: []OutputField{
					{Name: "Bytes", Operation: "sum", SplitAB: true},
				},
			},
			newSplitABWithNoBidiError("Bytes"),
		},

[jotak]

this is perhaps the weird part of these errors, I would say: having to include them as "%w", and conntrackInvalidError Error() returning an empty string

[ronensc]

@joel Thanks for reviewing and suggesting alternatives.
I agree with you about the %w and the empty string returned by Error().
This was the way I came up with to add an error message on the one hand and not having to check it in the unit tests on the other hand.
I have a different idea to get rid of %w: Instead of wrapping conntrackInvalidError, adding the message to it as a new field.

type conntrackInvalidError struct {
	msg                      error
	fieldGroupABOnlyOneIsSet bool
	splitABWithNoBidi        bool
	// ...
}

func (err conntrackInvalidError) Error() string {
	if err.msg != nil {
		return err.msg.Error()
	}
	return ""
}

The caller will be changed to:

if isGroupAEmpty != isGroupBEmpty { // XOR
	return conntrackInvalidError{fieldGroupABOnlyOneIsSet: true,
		msg: fmt.Errorf("only one of 'fieldGroupARef' and 'fieldGroupBRef' is set. They should both be set or both unset")}
}

And to make the test compare only the booleans and not the error messages, we can either implement Is() or Unwrap():

func (err conntrackInvalidError) Is(target error) bool {
	err.msg = nil
	return err == target
}

func (err conntrackInvalidError) Unwrap() error {
	err.msg = nil
	return err
}

https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/errors/wrap.go;l=40

@jotak if you stil don't like this idea, I'll go with your approach of multi error definition. 😄
It has the nice property that it also checks the additional error arguments. But, one downside is that the error message isn't close to the validation code. BTW, since the error objects don't require any special initialization, can't we avoid the newXXX() functions and instantiate them directly fieldGroupABOnlyOneIsSetError{}?

[jotak]

@ronensc your suggestion looks good to me 👍