diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 761b32ad..4651f30c 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -21,27 +21,11 @@ jobs: uname -a go version go env - - name: Vet - if: matrix.platform == 'ubuntu-latest' - run: go vet -v ./... - name: Lint - if: matrix.platform == 'ubuntu-latest' - run: | - export PATH=$PATH:$(go env GOPATH)/bin - go install golang.org/x/lint/golint@latest - golint -set_exit_status ./... - - name: staticcheck.io - if: matrix.platform == 'ubuntu-latest' - uses: dominikh/staticcheck-action@v1.3.0 + uses: golangci/golangci-lint-action@v3 with: - install-go: false - - name: gofumpt formatting - if: matrix.platform == 'ubuntu-latest' - run: | - export PATH=$PATH:$(go env GOPATH)/bin - go install mvdan.cc/gofumpt@latest - gofumpt -d . - [ -z "$(gofumpt -l .)" ] + skip-build-cache: true + skip-pkg-cache: true - name: Test run: go test -count=1 ./... - name: Test with -race diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..78d39c7d --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,14 @@ +run: + timeout: 10m +linters: + disable-all: true + enable: + - errcheck + - gofumpt + - gosimple + - govet + - ineffassign + - staticcheck + - stylecheck + - typecheck + - unused \ No newline at end of file diff --git a/Makefile b/Makefile index 443d62e0..a313f342 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,9 @@ CURRENTPATH = $(shell echo $(PWD)) WORKDIR = /src/github.com/getyourguide.com/istio-config-validator +GOLINT_CMD=golangci-lint +GO_FILES=$(shell find . -type f -regex ".*go") +.PHONY: run lint fix run: docker run -it --rm --name istio_config_validator \ @@ -8,3 +11,22 @@ run: -w ${WORKDIR} \ golang:1.21 \ go run cmd/istio-config-validator/main.go -t examples/ examples/ + +define check_linter + @if ! command -v $(GOLINT_CMD) > /dev/null; then\ + echo "$(GOLINT_CMD) must be installed to run linting: try brew install $(GOLINT_CMD)";\ + exit 1;\ + fi +endef + +lint: .golangci.yml ## Lint the code + +fix: ## Fix any issues found during linting + @$(call check_linter) + $(GOLINT_CMD) run --fix + +.golangci.yml: $(GO_FILES) + @$(call check_linter) + $(GOLINT_CMD) version + $(GOLINT_CMD) run + touch .golangci.yml \ No newline at end of file diff --git a/cmd/istio-config-validator/main.go b/cmd/istio-config-validator/main.go index d1feece9..0744c8bf 100644 --- a/cmd/istio-config-validator/main.go +++ b/cmd/istio-config-validator/main.go @@ -61,15 +61,18 @@ func main() { func getFiles(names []string) []string { var files []string for _, name := range names { - filepath.Walk(name, func(path string, info os.FileInfo, err error) error { + err := filepath.Walk(name, func(path string, info os.FileInfo, err error) error { if err != nil { - log.Fatal(err.Error()) + return err } if !info.IsDir() && isYaml(info) { files = append(files, path) } return nil }) + if err != nil { + log.Fatal(err.Error()) + } } return files } diff --git a/examples/destination.yml b/examples/destination.yml index d62d3b99..78dec93f 100644 --- a/examples/destination.yml +++ b/examples/destination.yml @@ -1,4 +1,4 @@ -# This file is to test skiping non virtual services files +# This file is to test skipping non virtual services files apiVersion: networking.istio.io/v1alpha3 kind: DestinationRule metadata: diff --git a/internal/pkg/parser/parser.go b/internal/pkg/parser/parser.go index 384a2ee0..33e62625 100644 --- a/internal/pkg/parser/parser.go +++ b/internal/pkg/parser/parser.go @@ -1,11 +1,11 @@ // Package parser is the package responsible for parsing test cases and istio configuration -// to be use on test assertionpackage parser +// to be used on test assertionpackage parser package parser import ( "fmt" - v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" + "istio.io/client-go/pkg/apis/networking/v1alpha3" ) // Parser contains the parsed files needed to run tests diff --git a/internal/pkg/parser/testcase.go b/internal/pkg/parser/testcase.go index 25ede957..b0ae2806 100644 --- a/internal/pkg/parser/testcase.go +++ b/internal/pkg/parser/testcase.go @@ -43,18 +43,23 @@ type TestCase struct { // Request define the crafted http request present in the test case file. type Request struct { - Authority []string `yaml:"authority"` - Method []string `yaml:"method"` - URI []string `yaml:"uri"` - Headers map[string]string `yaml:"headers"` + Authority []string `yaml:"authority"` + Method []string `yaml:"method"` + URI []string `yaml:"uri"` + // optional fields + Headers map[string]string `yaml:"headers"` + QueryParams map[string]string `yaml:"queryParams"` + Scheme string `yaml:"scheme"` } // Input contains the data structure which will be used to assert type Input struct { - Authority string - Method string - URI string - Headers map[string]string + Authority string + Method string + URI string + Headers map[string]string + Scheme string + QueryParams map[string]string } // Destination define the destination we should assert @@ -78,7 +83,7 @@ type Port struct { // {Authority:"example.com", Method: "OPTIONS"}, // } func (r *Request) Unfold() ([]Input, error) { - out := []Input{} + var out []Input if len(r.Authority) == 0 { return out, ErrEmptyAuthorityList @@ -98,7 +103,14 @@ func (r *Request) Unfold() ([]Input, error) { for _, auth := range r.Authority { for _, method := range r.Method { - out = append(out, Input{Authority: auth, Method: method, URI: u.Path, Headers: r.Headers}) + out = append(out, Input{ + Authority: auth, + Method: method, + URI: u.Path, + Headers: r.Headers, + QueryParams: r.QueryParams, + Scheme: r.Scheme, + }) } } } @@ -107,7 +119,7 @@ func (r *Request) Unfold() ([]Input, error) { } func parseTestCases(files []string) ([]*TestCase, error) { - out := []*TestCase{} + var out []*TestCase for _, file := range files { fileContent, err := os.ReadFile(file) @@ -137,7 +149,7 @@ func parseTestCases(files []string) ([]*TestCase, error) { yamlFile := &TestCaseYAML{} err = json.Unmarshal(jsonBytes, yamlFile) if err != nil { - slog.Debug("unmarshaling failed for file '%s': %w", file, err) + slog.Debug("unmarshalling failed for file '%s': %w", file, err) return []*TestCase{}, err } diff --git a/internal/pkg/parser/virtualservice.go b/internal/pkg/parser/virtualservice.go index 1039e05c..a489c14b 100644 --- a/internal/pkg/parser/virtualservice.go +++ b/internal/pkg/parser/virtualservice.go @@ -15,7 +15,7 @@ import ( ) func parseVirtualServices(files []string) ([]*v1alpha3.VirtualService, error) { - out := []*v1alpha3.VirtualService{} + var out []*v1alpha3.VirtualService for _, file := range files { fileContent, err := os.ReadFile(file) diff --git a/internal/pkg/unit/match_request.go b/internal/pkg/unit/match_request.go index 92bcbd32..ca8c777c 100644 --- a/internal/pkg/unit/match_request.go +++ b/internal/pkg/unit/match_request.go @@ -10,6 +10,37 @@ import ( "istio.io/api/networking/v1alpha3" ) +// MapMatcher checks map parameters, which requires executing multiple ExtendedStringMatch checks +type MapMatcher struct { + matchers map[string]*v1alpha3.StringMatch +} + +func (mm MapMatcher) Match(input map[string]string) (bool, error) { + for param, matcher := range mm.matchers { + value, exists := input[param] + if !exists { + return false, nil + } + + if matcher.GetMatchType() == nil { + // match type exact: "", and {} checks for presence in query and headers respectively + continue + } + + extendedMatcher := &ExtendedStringMatch{StringMatch: matcher} + matched, err := extendedMatcher.Match(value) + if err != nil { + return false, err + } + + if !matched { + return false, nil + } + } + + return true, nil +} + // ExtendedStringMatch copies istio ExtendedStringMatch definition and extends it to add helper methods. type ExtendedStringMatch struct { *v1alpha3.StringMatch @@ -26,12 +57,12 @@ func (sm *ExtendedStringMatch) Match(s string) (bool, error) { return true, nil } - switch { - case sm.GetExact() != "": - return sm.GetExact() == s, nil - case sm.GetPrefix() != "": + switch m := sm.GetMatchType().(type) { + case *v1alpha3.StringMatch_Exact: + return m.Exact == s, nil + case *v1alpha3.StringMatch_Prefix: return strings.HasPrefix(s, sm.GetPrefix()), nil - case sm.GetRegex() != "": + case *v1alpha3.StringMatch_Regex: // The rule will not match if only a subsequence of the string matches the regex. // https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routematch-safe-regex r, err := regexp.Compile("^" + sm.GetRegex() + "$") @@ -39,44 +70,45 @@ func (sm *ExtendedStringMatch) Match(s string) (bool, error) { return false, fmt.Errorf("could not compile regex %s: %v", sm.GetRegex(), err) } return r.MatchString(s), nil + default: + return false, fmt.Errorf("unknown matcher type %T", sm.GetMatchType()) } - return false, nil } // matchRequest takes an Input and evaluates against a HTTPMatchRequest block. It replicates // Istio VirtualService semantic returning true when ALL conditions within the block are true. // TODO: Add support for all fields within a match block. The ones supported today are: -// Authority, Uri, Method and Headers. +// Authority, Uri, Method, Headers, Scheme, and QueryParams. func matchRequest(input parser.Input, httpMatchRequest *v1alpha3.HTTPMatchRequest) (bool, error) { - authority := &ExtendedStringMatch{httpMatchRequest.Authority} uri := &ExtendedStringMatch{httpMatchRequest.Uri} - method := &ExtendedStringMatch{httpMatchRequest.Method} + if matched, err := uri.Match(input.URI); !matched || err != nil { + return false, err + } - for headerName, sm := range httpMatchRequest.Headers { - if _, ok := input.Headers[headerName]; !ok { - return false, nil - } - header := &ExtendedStringMatch{sm} - match, err := header.Match(input.Headers[headerName]) - if err != nil { - return false, err - } - if !match { - return false, nil - } + authority := &ExtendedStringMatch{httpMatchRequest.Authority} + if matched, err := authority.Match(input.Authority); !matched || err != nil { + return false, err } - uriMatch, err := uri.Match(input.URI) - if err != nil { + method := &ExtendedStringMatch{httpMatchRequest.Method} + if matched, err := method.Match(input.Method); !matched || err != nil { return false, err } - authorityMatch, err := authority.Match(input.Authority) - if err != nil { + + scheme := &ExtendedStringMatch{httpMatchRequest.Scheme} + if matched, err := scheme.Match(input.Scheme); !matched || err != nil { return false, err } - methodMatch, err := method.Match(input.Method) - if err != nil { + + headers := &MapMatcher{matchers: httpMatchRequest.Headers} + if matched, err := headers.Match(input.Headers); !matched || err != nil { return false, err } - return authorityMatch && uriMatch && methodMatch, nil + + query := &MapMatcher{matchers: httpMatchRequest.QueryParams} + if matched, err := query.Match(input.QueryParams); !matched || err != nil { + return false, err + } + + return true, nil } diff --git a/internal/pkg/unit/testcases/header/service.yml b/internal/pkg/unit/testcases/header/service.yml new file mode 100644 index 00000000..eddbf290 --- /dev/null +++ b/internal/pkg/unit/testcases/header/service.yml @@ -0,0 +1,31 @@ +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: example + namespace: example +spec: + gateways: + - mesh + hosts: + - www.example.com + - example.com + http: + - match: + - headers: + x-custom-header: + exact: ok + x-prefix-header: + prefix: start + route: + - destination: + host: has-header + - match: + - headers: + x-custom-header: {} + route: + - destination: + host: has-present-header + - match: + route: + - destination: + host: default diff --git a/internal/pkg/unit/testcases/header/tests.yml b/internal/pkg/unit/testcases/header/tests.yml new file mode 100644 index 00000000..5f9a7098 --- /dev/null +++ b/internal/pkg/unit/testcases/header/tests.yml @@ -0,0 +1,33 @@ +TestCases: + - description: match headers with a variety of values + wantMatch: true + request: + authority: ["www.example.com"] + method: ["GET"] + uri: ["/headers/v1"] + headers: + x-custom-header: ok + x-prefix-header: start-with-prefix + route: + - destination: + host: has-header + - description: match a header by presence + wantMatch: true + request: + authority: ["www.example.com"] + method: ["GET"] + uri: ["/headers/v1"] + headers: + x-custom-header: ok + route: + - destination: + host: has-present-header + - description: missing headers goes to default + wantMatch: true + request: + authority: ["www.example.com"] + method: ["GET"] + uri: ["/headers/v1"] + route: + - destination: + host: default diff --git a/internal/pkg/unit/testcases/query/service.yml b/internal/pkg/unit/testcases/query/service.yml new file mode 100644 index 00000000..c6d7d5c2 --- /dev/null +++ b/internal/pkg/unit/testcases/query/service.yml @@ -0,0 +1,42 @@ +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: example + namespace: example +spec: + gateways: + - mesh + hosts: + - www.example.com + - example.com + http: + - match: + # full match, all query params should be present + - uri: + prefix: /queries + queryParams: + version: + exact: + exactMatch: + exact: + name-matches + prefixMatch: + prefix: + prefixed- + route: + - destination: + host: full.query.svc.cluster.local + - match: + # partial match: version should be present + - uri: + prefix: /queries + queryParams: + version: + exact: + route: + - destination: + host: partial.query.svc.cluster.local + - match: + route: + - destination: + host: default.query.svc.cluster.local diff --git a/internal/pkg/unit/testcases/query/tests.yml b/internal/pkg/unit/testcases/query/tests.yml new file mode 100644 index 00000000..5d715d77 --- /dev/null +++ b/internal/pkg/unit/testcases/query/tests.yml @@ -0,0 +1,26 @@ +TestCases: + - description: match query parameters in multiple ways + wantMatch: true + request: + authority: ["www.example.com"] + method: ["GET"] + uri: ["/queries/v1"] + queryParams: + prefixMatch: prefixed-query-param + exactMatch: name-matches + version: + route: + - destination: + host: full.query.svc.cluster.local + - description: no match if a query parameter is missing + wantMatch: true + request: + authority: ["www.example.com"] + method: ["GET"] + uri: ["/queries/v1"] + queryParams: + exactMatch: name-matches + version: + route: + - destination: + host: partial.query.svc.cluster.local diff --git a/internal/pkg/unit/testcases/scheme/service.yml b/internal/pkg/unit/testcases/scheme/service.yml new file mode 100644 index 00000000..fed535ae --- /dev/null +++ b/internal/pkg/unit/testcases/scheme/service.yml @@ -0,0 +1,21 @@ +apiVersion: networking.istio.io/v1alpha3 +kind: VirtualService +metadata: + name: scheme + namespace: scheme +spec: + gateways: + - mesh + hosts: + - scheme.com + http: + - match: + - scheme: + exact: https + route: + - destination: + host: has-https + - match: + route: + - destination: + host: http diff --git a/internal/pkg/unit/testcases/scheme/tests.yml b/internal/pkg/unit/testcases/scheme/tests.yml new file mode 100644 index 00000000..2fd0622d --- /dev/null +++ b/internal/pkg/unit/testcases/scheme/tests.yml @@ -0,0 +1,21 @@ +TestCases: + - description: if scheme matches https send to https endpoint + wantMatch: true + request: + scheme: https + authority: ["scheme.com"] + method: ["GET"] + uri: ["/scheme/v1"] + route: + - destination: + host: has-https + - description: if scheme does not match https send to http endpoint + wantMatch: true + request: + scheme: ftp + authority: ["scheme.com"] + method: ["GET"] + uri: ["/scheme/v1"] + route: + - destination: + host: http diff --git a/internal/pkg/unit/unit.go b/internal/pkg/unit/unit.go index f85d66c1..c752e592 100644 --- a/internal/pkg/unit/unit.go +++ b/internal/pkg/unit/unit.go @@ -1,5 +1,5 @@ // Package unit contains logic to run the unit tests against istio configuration. -// It intends to replicates istio logic when it comes to matching requests and defining its destinations. +// It intends to replicate istio logic when it comes to matching requests and defining its destinations. // Once the destinations are found for a given test case, it will try to assert with the expected results. package unit @@ -9,61 +9,94 @@ import ( "github.com/getyourguide/istio-config-validator/internal/pkg/parser" networkingv1alpha3 "istio.io/api/networking/v1alpha3" - v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" + "istio.io/client-go/pkg/apis/networking/v1alpha3" ) +type Formatter interface { + Pass(input parser.Input) string + Fail(input parser.Input) string +} + +type TestRunner struct { + TestCases []*parser.TestCase + VirtualServices []*v1alpha3.VirtualService + Format Formatter +} + +type defaultFormat struct{} + +func (d defaultFormat) Fail(input parser.Input) string { + return fmt.Sprintf("FAIL input:[%v]", input) +} + +func (d defaultFormat) Pass(input parser.Input) string { + return fmt.Sprintf("Pass input:[%v]", input) +} + +var defaultFormatter Formatter = defaultFormat{} + // Run is the entrypoint to run all unit tests defined in test cases func Run(testfiles, configfiles []string) ([]string, []string, error) { - var summary, details []string parsed, err := parser.New(testfiles, configfiles) + runner := TestRunner{TestCases: parsed.TestCases, VirtualServices: parsed.VirtualServices} if err != nil { - return summary, details, err + return nil, nil, err } + summary, details, err := runner.Run() + summary = append(summary, fmt.Sprintf(" - %d testfiles, %d configfiles", len(testfiles), len(configfiles))) + return summary, details, err +} +// Run executes the TestRunner +func (tr *TestRunner) Run() ([]string, []string, error) { + if tr.Format == nil { + tr.Format = defaultFormatter + } + + var summary, details []string inputCount := 0 - for _, testCase := range parsed.TestCases { + for _, testCase := range tr.TestCases { details = append(details, "running test: "+testCase.Description) inputs, err := testCase.Request.Unfold() if err != nil { return summary, details, err } for _, input := range inputs { - route, err := GetRoute(input, parsed.VirtualServices) + route, err := GetRoute(input, tr.VirtualServices) if err != nil { - details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) + details = append(details, tr.Format.Fail(input)) return summary, details, fmt.Errorf("error getting destinations: %v", err) } if reflect.DeepEqual(route.Route, testCase.Route) != testCase.WantMatch { - details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) + details = append(details, tr.Format.Fail(input)) return summary, details, fmt.Errorf("destination missmatch=%v, want %v, rule matched: %v", route.Route, testCase.Route, route.Match) } if testCase.Rewrite != nil { if reflect.DeepEqual(route.Rewrite, testCase.Rewrite) != testCase.WantMatch { - details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) + details = append(details, tr.Format.Fail(input)) return summary, details, fmt.Errorf("rewrite missmatch=%v, want %v, rule matched: %v", route.Rewrite, testCase.Rewrite, route.Match) } } if testCase.Fault != nil { if reflect.DeepEqual(route.Fault, testCase.Fault) != testCase.WantMatch { - details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) + details = append(details, tr.Format.Fail(input)) return summary, details, fmt.Errorf("fault missmatch=%v, want %v, rule matched: %v", route.Fault, testCase.Fault, route.Match) } } if testCase.Headers != nil { if reflect.DeepEqual(route.Headers, testCase.Headers) != testCase.WantMatch { - details = append(details, fmt.Sprintf("FAIL input:[%v]", input)) + details = append(details, tr.Format.Fail(input)) return summary, details, fmt.Errorf("headers missmatch=%v, want %v, rule matched: %v", route.Headers, testCase.Headers, route.Match) } } - details = append(details, fmt.Sprintf("PASS input:[%v]", input)) + details = append(details, tr.Format.Pass(input)) } inputCount += len(inputs) details = append(details, "===========================") } 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(tr.TestCases), inputCount)) return summary, details, nil } @@ -80,9 +113,11 @@ func GetRoute(input parser.Input, virtualServices []*v1alpha3.VirtualService) (* return httpRoute, nil } for _, matchBlock := range httpRoute.Match { - if match, err := matchRequest(input, matchBlock); err != nil { + match, err := matchRequest(input, matchBlock) + if err != nil { return &networkingv1alpha3.HTTPRoute{}, err - } else if match { + } + if match { return httpRoute, nil } } diff --git a/internal/pkg/unit/unit_test.go b/internal/pkg/unit/unit_test.go index 37f85d1f..b074c99a 100644 --- a/internal/pkg/unit/unit_test.go +++ b/internal/pkg/unit/unit_test.go @@ -1,12 +1,18 @@ package unit import ( + "fmt" + "os" + "path" "reflect" + "strings" "testing" "github.com/getyourguide/istio-config-validator/internal/pkg/parser" + "gopkg.in/yaml.v3" networkingv1alpha3 "istio.io/api/networking/v1alpha3" v1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3" + "istio.io/pkg/log" ) func TestRun(t *testing.T) { @@ -197,3 +203,66 @@ func TestGetRoute(t *testing.T) { }) } } + +type yamlFormatter struct{} + +var yamlFormat Formatter = yamlFormatter{} + +func (y yamlFormatter) Pass(input parser.Input) string { + b, err := yaml.Marshal(input) + if err != nil { + log.Warn(fmt.Sprintf("failed to parse input to YAML, fellback to default format: %v", err)) + return defaultFormatter.Pass(input) + } + return fmt.Sprintf("Pass, input:\n%s", string(b)) +} + +func (y yamlFormatter) Fail(input parser.Input) string { + b, err := yaml.Marshal(input) + if err != nil { + log.Warn(fmt.Sprintf("failed to parse input to YAML, fellback to default format: %v", err)) + return defaultFormatter.Fail(input) + } + return fmt.Sprintf("Fail, input:\n%s", string(b)) +} + +func TestFromFile(t *testing.T) { + workdir, err := os.Getwd() + if err != nil { + t.Error(err) + } + testCasesRoot := path.Join(workdir, "testcases") + entries, err := os.ReadDir(testCasesRoot) + if err != nil { + t.Error(err) + } + + for _, entry := range entries { + testRoot := path.Join(testCasesRoot, entry.Name()) + tests := []string{path.Join(testRoot, "tests.yml")} + services := []string{path.Join(testRoot, "service.yml")} + parsed, err := parser.New(tests, services) + if err != nil { + t.Error(err) + } + + t.Run(entry.Name(), func(t *testing.T) { + for _, testCase := range parsed.TestCases { + runner := &TestRunner{ + TestCases: []*parser.TestCase{testCase}, + VirtualServices: parsed.VirtualServices, + Format: yamlFormat, + } + t.Run(testCase.Description, func(t *testing.T) { + summary, details, err := runner.Run() + t.Log(strings.Join(summary, "\n")) + t.Log(strings.Join(details, "\n")) + if err != nil { + t.Error(err) + } + }) + } + }) + + } +}