parser: add strict mode (#413)

* parser: strict parsing manifests and tests

Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>

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())

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)
 	}

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)
 
 			}
 

internal/pkg/parser/testcase_test.go

@@ -4,34 +4,38 @@ 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
 			}
 		}
 		if !testPass {
 			t.Errorf("could not find expected description:'%v'", expected.Description)
 		}
-
 	}
 }
 

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

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

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
 }

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")
+}