Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NOREF] add support for queryParams and scheme #354

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/destination.yml
Original file line number Diff line number Diff line change
@@ -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:
Expand Down
4 changes: 2 additions & 2 deletions internal/pkg/parser/parser.go
Original file line number Diff line number Diff line change
@@ -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
Expand Down
36 changes: 24 additions & 12 deletions internal/pkg/parser/testcase.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate field for the parameters or can we use the uri and parse from it?

Also, the same parameter can be set multiple times and interpreted differently by web frameworks as there is no apparent standard, this would be another reason not to use map[string]string

https://go.dev/play/p/dcExJ0RlFYT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed will keep as is for now - the envoy behaviour is to check the first query param

}

// Destination define the destination we should assert
Expand All @@ -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
Expand All @@ -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,
})
}
}
}
Expand All @@ -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)
Expand Down Expand Up @@ -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

}
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/parser/virtualservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
88 changes: 60 additions & 28 deletions internal/pkg/unit/match_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,37 @@
"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) {

Check failure on line 18 in internal/pkg/unit/match_request.go

View workflow job for this annotation

GitHub Actions / test (1.21.x, ubuntu-latest)

exported method MapMatcher.Match should have comment or be unexported
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
Expand All @@ -26,57 +57,58 @@
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() + "$")
if err != nil {
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())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an error here is a behaviour change - could also be a log

}
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
}
31 changes: 31 additions & 0 deletions internal/pkg/unit/testcases/header/service.yml
Original file line number Diff line number Diff line change
@@ -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
33 changes: 33 additions & 0 deletions internal/pkg/unit/testcases/header/tests.yml
Original file line number Diff line number Diff line change
@@ -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
42 changes: 42 additions & 0 deletions internal/pkg/unit/testcases/query/service.yml
Original file line number Diff line number Diff line change
@@ -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
26 changes: 26 additions & 0 deletions internal/pkg/unit/testcases/query/tests.yml
Original file line number Diff line number Diff line change
@@ -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
21 changes: 21 additions & 0 deletions internal/pkg/unit/testcases/scheme/service.yml
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading