diff --git a/cmd/istio-config-validator/main.go b/cmd/istio-config-validator/main.go index 8ce9a0e3..eb32abcf 100644 --- a/cmd/istio-config-validator/main.go +++ b/cmd/istio-config-validator/main.go @@ -30,6 +30,8 @@ func main() { var testCaseParams multiValueFlag flag.Var(&testCaseParams, "t", "Testcase files/folders") summaryOnly := flag.Bool("s", false, "show only summary of tests (in case of failures full details are shown)") + strict := flag.Bool("strict", false, "fail on unknown fields") + flag.Parse() istioConfigFiles := getFiles(flag.Args()) testCaseFiles := getFiles(testCaseParams) @@ -45,7 +47,7 @@ func main() { os.Exit(1) } - summary, details, err := unit.Run(testCaseFiles, istioConfigFiles) + summary, details, err := unit.Run(testCaseFiles, istioConfigFiles, *strict) if err != nil { fmt.Println(strings.Join(details, "\n")) log.Fatal(err.Error()) diff --git a/internal/pkg/istio-router-check/cmd/root.go b/internal/pkg/istio-router-check/cmd/root.go index fef4c079..6b732f01 100644 --- a/internal/pkg/istio-router-check/cmd/root.go +++ b/internal/pkg/istio-router-check/cmd/root.go @@ -172,7 +172,8 @@ func (c *RootCommand) prepareTests(ctx context.Context) error { if err != nil { return fmt.Errorf("could not read directory %s: %w", c.TestDir, err) } - testCases, err := parser.ParseTestCases(oldFiles) + strict := true + testCases, err := parser.ParseTestCases(oldFiles, strict) if err != nil { return fmt.Errorf("parsing testcases failed: %w", err) } diff --git a/internal/pkg/parser/parser.go b/internal/pkg/parser/parser.go deleted file mode 100644 index 38ee7144..00000000 --- a/internal/pkg/parser/parser.go +++ /dev/null @@ -1,33 +0,0 @@ -// Package parser is the package responsible for parsing test cases and istio configuration -// to be use on test assertionpackage parser -package parser - -import ( - "fmt" - - v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" -) - -// Parser contains the parsed files needed to run tests -type Parser struct { - TestCases []*TestCase - VirtualServices []*v1alpha3.VirtualService -} - -// New parses and loads the testcases and istio configuration files -func New(testfiles, configfiles []string) (*Parser, error) { - testCases, err := ParseTestCases(testfiles) - if err != nil { - return nil, fmt.Errorf("parsing testcases failed: %w", err) - } - - virtualServices, err := ParseVirtualServices(configfiles) - if err != nil { - return nil, fmt.Errorf("parsing virtualservices failed: %w", err) - } - parser := &Parser{ - TestCases: testCases, - VirtualServices: virtualServices, - } - return parser, nil -} diff --git a/internal/pkg/parser/testcase.go b/internal/pkg/parser/testcase.go index b3141273..ab0567d1 100644 --- a/internal/pkg/parser/testcase.go +++ b/internal/pkg/parser/testcase.go @@ -1,6 +1,7 @@ package parser import ( + "bytes" "encoding/json" "errors" "fmt" @@ -107,20 +108,18 @@ func (r *Request) Unfold() ([]Input, error) { return out, nil } -func ParseTestCases(files []string) ([]*TestCase, error) { +func ParseTestCases(files []string, strict bool) ([]*TestCase, error) { out := []*TestCase{} for _, file := range files { fileContent, err := os.ReadFile(file) if err != nil { - return []*TestCase{}, fmt.Errorf("reading file '%s' failed: %w", file, err) + return nil, fmt.Errorf("reading file %q failed: %w", file, err) } decoder := yamlV3.NewDecoder(strings.NewReader(string(fileContent))) - for { var testcaseInterface interface{} - if err = decoder.Decode(&testcaseInterface); err != nil { if errors.Is(err, io.EOF) { break @@ -132,14 +131,17 @@ func ParseTestCases(files []string) ([]*TestCase, error) { jsonBytes, err := json.Marshal(testcaseInterface) if err != nil { - return []*TestCase{}, fmt.Errorf("yamltojson conversion failed for file '%s': %w", file, err) + return nil, fmt.Errorf("yamltojson conversion failed for file %q: %w", file, err) + } + jsonDecoder := json.NewDecoder(bytes.NewReader(jsonBytes)) + if strict { + jsonDecoder.DisallowUnknownFields() } - yamlFile := &TestCaseYAML{} - err = json.Unmarshal(jsonBytes, yamlFile) - if err != nil { - slog.Debug("unmarshaling failed for file '%s': %w", file, err) - return []*TestCase{}, err + var yamlFile TestCaseYAML + if err := jsonDecoder.Decode(&yamlFile); err != nil { + slog.Debug("unmarshaling failed for file %q: %w", file, err) + return nil, fmt.Errorf("unmarshaling failed for file %q: %w", file, err) } diff --git a/internal/pkg/parser/testcase_test.go b/internal/pkg/parser/testcase_test.go index 7b7962ff..1d48b18e 100644 --- a/internal/pkg/parser/testcase_test.go +++ b/internal/pkg/parser/testcase_test.go @@ -4,26 +4,31 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestUnknownField(t *testing.T) { + testsFiles := []string{"testdata/invalid_test.yml"} + _, err := ParseTestCases(testsFiles, true) + require.ErrorContains(t, err, "json: unknown field") + + _, err = ParseTestCases(testsFiles, false) + require.NoError(t, err) +} + func TestParseTestCases(t *testing.T) { expectedTestCases := []*TestCase{ {Description: "happy path users"}, {Description: "Partner service only accepts GET or OPTIONS"}, } testcasefiles := []string{"../../../examples/virtualservice_test.yml"} - configfiles := []string{"../../../examples/virtualservice.yml"} - parser, err := New(testcasefiles, configfiles) - if err != nil { - t.Errorf("error getting test cases %v", err) - } - if len(parser.TestCases) == 0 { - t.Error("test cases are empty") - } + testCases, err := ParseTestCases(testcasefiles, false) + require.NoError(t, err) + require.NotEmpty(t, testCases) for _, expected := range expectedTestCases { testPass := false - for _, out := range parser.TestCases { + for _, out := range testCases { if expected.Description == out.Description { testPass = true } @@ -31,7 +36,6 @@ func TestParseTestCases(t *testing.T) { if !testPass { t.Errorf("could not find expected description:'%v'", expected.Description) } - } } diff --git a/internal/pkg/parser/testdata/invalid_test.yml b/internal/pkg/parser/testdata/invalid_test.yml new file mode 100644 index 00000000..5a0e62bb --- /dev/null +++ b/internal/pkg/parser/testdata/invalid_test.yml @@ -0,0 +1,16 @@ +testCases: + - description: a test + unknownField: true # not a valid field + wantMatch: true + request: + authority: + - www.example.com + method: + - GET + uri: + - /users + route: + - destination: + host: users.users.svc.cluster.local + port: + number: 80 diff --git a/internal/pkg/parser/testdata/invalid_vs.yml b/internal/pkg/parser/testdata/invalid_vs.yml new file mode 100644 index 00000000..e6d3ea70 --- /dev/null +++ b/internal/pkg/parser/testdata/invalid_vs.yml @@ -0,0 +1,16 @@ +apiVersion: networking.istio.io/v1 +kind: VirtualService +metadata: + name: example + namespace: example +spec: + hosts: www.example.com # invalid type + http: + - match: + - uri: + regex: /users(/.*)? + route: + - destination: + host: users.users.svc.cluster.local + port: + number: 80 diff --git a/internal/pkg/parser/virtualservice.go b/internal/pkg/parser/virtualservice.go index 4344f6f5..d1ecdb4a 100644 --- a/internal/pkg/parser/virtualservice.go +++ b/internal/pkg/parser/virtualservice.go @@ -1,70 +1,40 @@ package parser import ( - "encoding/json" "fmt" - "io" "os" - "strings" - "go.uber.org/zap/zapcore" - "golang.org/x/exp/slog" - yamlV3 "gopkg.in/yaml.v3" + networking "istio.io/api/networking/v1alpha3" v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "istio.io/istio/pilot/pkg/config/kube/crd" + "istio.io/istio/pkg/config/schema/gvk" ) func ParseVirtualServices(files []string) ([]*v1alpha3.VirtualService, error) { out := []*v1alpha3.VirtualService{} - for _, file := range files { fileContent, err := os.ReadFile(file) if err != nil { - return []*v1alpha3.VirtualService{}, fmt.Errorf("reading file '%s' failed: %w", file, err) + return nil, fmt.Errorf("reading file %q failed: %w", file, err) } - - decoder := yamlV3.NewDecoder(strings.NewReader(string(fileContent))) - - for { - // Reading into interface first. Decoding directly into struct does not work for Uri StringMatch types - var vsInterface interface{} - - if err = decoder.Decode(&vsInterface); err != nil { - // We've read every document in the file and we can break out - if err == io.EOF { - break - } - - slog.Debug("error while trying to unmarshal into interface", zapcore.Field{Key: "file", Type: zapcore.StringType, String: file}) - return out, fmt.Errorf("error while trying to unmarshal into interface (%s): %w", file, err) - } - - jsonBytes, err := json.Marshal(vsInterface) - if err != nil { - slog.Debug("error while trying to marshal to json", zapcore.Field{Key: "file", Type: zapcore.StringType, String: file}) - return out, fmt.Errorf("error while trying to marshal to json (%s): %w", file, err) - } - - meta := &v1.TypeMeta{} - if err = json.Unmarshal(jsonBytes, meta); err != nil { - slog.Debug("error extracting the metadata of the virtualservice", zapcore.Field{Key: "file", Type: zapcore.StringType, String: file}) + configs, _, err := crd.ParseInputs(string(fileContent)) + if err != nil { + return nil, fmt.Errorf("failed to parse CRD %q: %w", file, err) + } + for _, c := range configs { + if c.Meta.GroupVersionKind != gvk.VirtualService { continue } - - if meta.Kind != "VirtualService" { - slog.Debug("file is not Kind VirtualService", zapcore.Field{Key: "file", Type: zapcore.StringType, String: file}) - continue + spec, ok := c.Spec.(*networking.VirtualService) + if !ok { + return nil, fmt.Errorf("failed to convert spec in %q to VirtualService: %w", file, err) } - - virtualService := &v1alpha3.VirtualService{} - if err = json.Unmarshal(jsonBytes, virtualService); err != nil { - slog.Debug("error while trying to unmarshal virtualservice", zapcore.Field{Key: "file", Type: zapcore.StringType, String: file}) - return out, fmt.Errorf("error while trying to unmarshal virtualservice (%s): %w", file, err) + vs := &v1alpha3.VirtualService{ + ObjectMeta: c.ToObjectMeta(), + Spec: *spec, //nolint as deep copying mess up with reflect.DeepEqual comparison. } - - out = append(out, virtualService) + out = append(out, vs) } } - return out, nil } diff --git a/internal/pkg/parser/virtualservice_test.go b/internal/pkg/parser/virtualservice_test.go index fa48ce6a..70425047 100644 --- a/internal/pkg/parser/virtualservice_test.go +++ b/internal/pkg/parser/virtualservice_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" networkingv1alpha3 "istio.io/api/networking/v1alpha3" v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" ) @@ -12,18 +13,13 @@ func TestParseVirtualServices(t *testing.T) { expectedTestCases := []*v1alpha3.VirtualService{{Spec: networkingv1alpha3.VirtualService{ Hosts: []string{"www.example.com", "example.com"}, }}} - testcasefiles := []string{"../../../examples/virtualservice_test.yml"} configfiles := []string{"../../../examples/virtualservice.yml"} - parser, err := New(testcasefiles, configfiles) - if err != nil { - t.Errorf("error getting test cases %v", err) - } - if len(parser.VirtualServices) == 0 { - t.Error("virtualservices is empty") - } + virtualServices, err := ParseVirtualServices(configfiles) + require.NoError(t, err) + require.NotEmpty(t, virtualServices) for _, expected := range expectedTestCases { - for _, out := range parser.VirtualServices { + for _, out := range virtualServices { assert.ElementsMatch(t, expected.Spec.Hosts, out.Spec.Hosts) } } @@ -33,22 +29,22 @@ func TestParseMultipleVirtualServices(t *testing.T) { expectedTestCases := []*v1alpha3.VirtualService{{Spec: networkingv1alpha3.VirtualService{ Hosts: []string{"www.example.com", "example.com"}, }}} - testcasefiles := []string{"../../../examples/virtualservice_test.yml"} + configfiles := []string{"../../../examples/multidocument_virtualservice.yml"} - parser, err := New(testcasefiles, configfiles) - if err != nil { - t.Errorf("error getting test cases %v", err) - } - if len(parser.VirtualServices) == 0 { - t.Error("virtualservices is empty") - } - if len(parser.VirtualServices) < 2 { - t.Error("did not parse all virtualservices in file") - } + virtualServices, err := ParseVirtualServices(configfiles) + require.NoError(t, err) + require.NotEmpty(t, virtualServices) + require.GreaterOrEqual(t, len(virtualServices), 2) for _, expected := range expectedTestCases { - for _, out := range parser.VirtualServices { + for _, out := range virtualServices { assert.ElementsMatch(t, expected.Spec.Hosts, out.Spec.Hosts) } } } + +func TestVirtualServiceUnknownFields(t *testing.T) { + vsFiles := []string{"testdata/invalid_vs.yml"} + _, err := ParseVirtualServices(vsFiles) + require.ErrorContains(t, err, "cannot parse proto message") +} diff --git a/internal/pkg/unit/unit.go b/internal/pkg/unit/unit.go index 7ff31e0a..9957da4d 100644 --- a/internal/pkg/unit/unit.go +++ b/internal/pkg/unit/unit.go @@ -14,15 +14,21 @@ import ( ) // Run is the entrypoint to run all unit tests defined in test cases -func Run(testfiles, configfiles []string) ([]string, []string, error) { +func Run(testfiles, configfiles []string, strict bool) ([]string, []string, error) { var summary, details []string - parsed, err := parser.New(testfiles, configfiles) + + testCases, err := parser.ParseTestCases(testfiles, strict) + if err != nil { + return nil, nil, fmt.Errorf("parsing testcases failed: %w", err) + } + + virtualServices, err := parser.ParseVirtualServices(configfiles) if err != nil { - return summary, details, err + return nil, nil, fmt.Errorf("parsing virtualservices failed: %w", err) } inputCount := 0 - for _, testCase := range parsed.TestCases { + for _, testCase := range testCases { details = append(details, "running test: "+testCase.Description) inputs, err := testCase.Request.Unfold() if err != nil { @@ -30,7 +36,7 @@ func Run(testfiles, configfiles []string) ([]string, []string, error) { } for _, input := range inputs { checkHosts := true - route, err := GetRoute(input, parsed.VirtualServices, checkHosts) + route, err := GetRoute(input, virtualServices, checkHosts) if err != nil { details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) return summary, details, fmt.Errorf("error getting destinations: %v", err) @@ -44,7 +50,7 @@ func Run(testfiles, configfiles []string) ([]string, []string, error) { details = append(details, fmt.Sprintf("PASS input:[%v]", input)) } if testCase.Route != nil { - vs, err := GetDelegatedVirtualService(route.Delegate, parsed.VirtualServices) + vs, err := GetDelegatedVirtualService(route.Delegate, virtualServices) if err != nil { details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) return summary, details, fmt.Errorf("error getting delegate virtual service: %v", err) @@ -89,7 +95,7 @@ func Run(testfiles, configfiles []string) ([]string, []string, error) { } summary = append(summary, "Test summary:") summary = append(summary, fmt.Sprintf(" - %d testfiles, %d configfiles", len(testfiles), len(configfiles))) - summary = append(summary, fmt.Sprintf(" - %d testcases with %d inputs passed", len(parsed.TestCases), inputCount)) + summary = append(summary, fmt.Sprintf(" - %d testcases with %d inputs passed", len(testCases), inputCount)) return summary, details, nil } diff --git a/internal/pkg/unit/unit_test.go b/internal/pkg/unit/unit_test.go index f259faec..7f56223b 100644 --- a/internal/pkg/unit/unit_test.go +++ b/internal/pkg/unit/unit_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/getyourguide/istio-config-validator/internal/pkg/parser" + "github.com/stretchr/testify/require" networkingv1alpha3 "istio.io/api/networking/v1alpha3" v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -13,21 +14,17 @@ import ( func TestRun(t *testing.T) { testcasefiles := []string{"../../../examples/virtualservice_test.yml"} configfiles := []string{"../../../examples/virtualservice.yml"} - - _, _, err := Run(testcasefiles, configfiles) - if err != nil { - t.Error(err) - } + var strict bool + _, _, err := Run(testcasefiles, configfiles, strict) + require.NoError(t, err) } func TestRunDelegate(t *testing.T) { testcasefiles := []string{"../../../examples/virtualservice_delegate_test.yml"} configfiles := []string{"../../../examples/delegate_virtualservice.yml"} - - _, _, err := Run(testcasefiles, configfiles) - if err != nil { - t.Error(err) - } + var strict bool + _, _, err := Run(testcasefiles, configfiles, strict) + require.NoError(t, err) } func TestGetRoute(t *testing.T) {