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.

Sign up for GitHub

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

Jump to bottom

xds/internal/xdsclient: Add support for String Matcher Header Matcher in RDS #6313

Merged
merged 2 commits into from May 25, 2023
Merged

xds/internal/xdsclient: Add support for String Matcher Header Matcher in RDS #6313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1722c3
Add support for a String Matcher Header Matcher
zasweq May 24, 2023
fd57cfb
Responded to Doug's comments
zasweq May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions internal/xds/matcher/matcher_header.go
View file
Open in desktop
Expand Up @@ -241,3 +241,34 @@ func (hcm *HeaderContainsMatcher) Match(md metadata.MD) bool {
func (hcm *HeaderContainsMatcher) String() string {
return fmt.Sprintf("headerContains:%v%v", hcm.key, hcm.contains)
}

// HeaderStringMatcher matches on whether the header value matches against the
// StringMatcher specified.
type HeaderStringMatcher struct {
key string
stringMatcher StringMatcher
invert bool
}

// NewHeaderStringMatcher returns a new HeaderStringMatcher.
func NewHeaderStringMatcher(key string, sm StringMatcher, invert bool) *HeaderStringMatcher {
return &HeaderStringMatcher{
key: key,
stringMatcher: sm,
invert: invert,
}
}

// Match returns whether the passed in HTTP Headers match according to the
// specified StringMatcher.
func (hsm *HeaderStringMatcher) Match(md metadata.MD) bool {
v, ok := mdValuesFromOutgoingCtx(md, hsm.key)
if !ok {
return false
}
return hsm.stringMatcher.Match(v) != hsm.invert
Copy link
Member

Choose a reason for hiding this comment

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

The behavior here is unclear in some cases:

  1. If a header is missing, should it always report false? Or should it report true if invert=true?
  2. If a header value is "", is that distinct from the header being missing, in any way?
  3. Is the check immediately above designed to only detect an error in usage? Because it doesn't consider (1) at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my chat thread. I think this is WAI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. See Eric's explanation here: HeaderMatcher.invert_match should invert the operation, not the result #4896
  2. Yes, a set empty header string is logically distinct from it not being presence
  3. No, this is part of the logic for the header matcher (not an error in usage, a header matcher can be called with metadata not containing the header key set in the matcher), as per explanation in 1

}

func (hsm *HeaderStringMatcher) String() string {
return fmt.Sprintf("headerString:%v:%v", hsm.key, hsm.stringMatcher)
}
80 changes: 80 additions & 0 deletions internal/xds/matcher/matcher_header_test.go
View file
Open in desktop
Expand Up @@ -467,3 +467,83 @@ func TestHeaderSuffixMatcherMatch(t *testing.T) {
})
}
}

func TestHeaderStringMatch(t *testing.T) {
tests := []struct {
name string
key string
sm StringMatcher
invert bool
md metadata.MD
want bool
}{
{
name: "should-match",
key: "th",
sm: StringMatcher{
exactMatch: newStringP("tv"),
},
invert: false,
md: metadata.Pairs("th", "tv"),
want: true,
},
{
name: "not match",
key: "th",
sm: StringMatcher{
containsMatch: newStringP("tv"),
},
invert: false,
md: metadata.Pairs("th", "not-match"),
want: false,
},
{
name: "invert string match",
key: "th",
sm: StringMatcher{
containsMatch: newStringP("tv"),
},
invert: true,
md: metadata.Pairs("th", "not-match"),
want: true,
},
{
name: "header missing",
key: "th",
sm: StringMatcher{
containsMatch: newStringP("tv"),
},
invert: false,
md: metadata.Pairs("not-specified-key", "not-match"),
want: false,
},
{
name: "header missing invert true",
key: "th",
sm: StringMatcher{
containsMatch: newStringP("tv"),
},
invert: true,
md: metadata.Pairs("not-specified-key", "not-match"),
want: false,
},
{
name: "header empty string invert",
key: "th",
sm: StringMatcher{
containsMatch: newStringP("tv"),
},
invert: true,
md: metadata.Pairs("th", ""),
want: true,
},
Copy link
Member

Choose a reason for hiding this comment

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

Please add test cases that cover the unusual corner cases mentioned above once we determine the correct behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added 3 test cases, invert and not invert for missing, and invert for empty string wanting true.

}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
hsm := NewHeaderStringMatcher(test.key, test.sm, test.invert)
if got := hsm.Match(test.md); got != test.want {
t.Errorf("match() = %v, want %v", got, test.want)
}
})
}
}
2 changes: 2 additions & 0 deletions xds/internal/xdsclient/xdsresource/matcher.go
View file
Open in desktop
Expand Up @@ -59,6 +59,8 @@ func RouteToMatcher(r *Route) (*CompositeMatcher, error) {
matcherT = matcher.NewHeaderRangeMatcher(h.Name, h.RangeMatch.Start, h.RangeMatch.End, invert)
case h.PresentMatch != nil:
matcherT = matcher.NewHeaderPresentMatcher(h.Name, *h.PresentMatch, invert)
case h.StringMatch != nil:
matcherT = matcher.NewHeaderStringMatcher(h.Name, *h.StringMatch, invert)
default:
return nil, fmt.Errorf("illegal route: missing header_match_specifier")
}
Expand Down
1 change: 1 addition & 0 deletions xds/internal/xdsclient/xdsresource/type_rds.go
View file
Open in desktop
Expand Up @@ -171,6 +171,7 @@ type HeaderMatcher struct {
SuffixMatch *string
RangeMatch *Int64Range
PresentMatch *bool
StringMatch *matcher.StringMatcher
}

// Int64Range is a range for header range match.
Expand Down
12 changes: 10 additions & 2 deletions xds/internal/xdsclient/xdsresource/unmarshal_rds.go
View file
Open in desktop
Expand Up @@ -24,13 +24,15 @@ import (
"strings"
"time"

v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3"
"github.com/golang/protobuf/proto"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/xds/matcher"
"google.golang.org/grpc/xds/internal/clusterspecifier"
"google.golang.org/protobuf/types/known/anypb"

v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
v3typepb "github.com/envoyproxy/go-control-plane/envoy/type/v3"
)

func unmarshalRouteConfigResource(r *anypb.Any) (string, RouteConfigUpdate, error) {
Expand Down Expand Up @@ -273,6 +275,12 @@ func routesProtoToSlice(routes []*v3routepb.Route, csps map[string]clusterspecif
header.PrefixMatch = &ht.PrefixMatch
case *v3routepb.HeaderMatcher_SuffixMatch:
header.SuffixMatch = &ht.SuffixMatch
case *v3routepb.HeaderMatcher_StringMatch:
sm, err := matcher.StringMatcherFromProto(ht.StringMatch)
if err != nil {
return nil, nil, fmt.Errorf("route %+v has an invalid string matcher: %v", err, ht.StringMatch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't correct formatting, I need to fix.

}
header.StringMatch = &sm
default:
return nil, nil, fmt.Errorf("route %+v has an unrecognized header matcher: %+v", r, ht)
}
Expand Down
47 changes: 47 additions & 0 deletions xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ import (
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/pretty"
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/internal/xds/matcher"
"google.golang.org/grpc/xds/internal/clusterspecifier"
"google.golang.org/grpc/xds/internal/httpfilter"
"google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version"
Expand Down Expand Up @@ -923,6 +924,7 @@ func (s) TestUnmarshalRouteConfig(t *testing.T) {
}

func (s) TestRoutesProtoToSlice(t *testing.T) {
sm, _ := matcher.StringMatcherFromProto(&v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "tv"}})
var (
goodRouteWithFilterConfigs = func(cfgs map[string]*anypb.Any) []*v3routepb.Route {
// Sets per-filter config in cluster "B" and in the route.
Expand Down Expand Up @@ -1085,6 +1087,51 @@ func (s) TestRoutesProtoToSlice(t *testing.T) {
}},
wantErr: false,
},
{
name: "good with string matcher",
routes: []*v3routepb.Route{
{
Match: &v3routepb.RouteMatch{
PathSpecifier: &v3routepb.RouteMatch_SafeRegex{SafeRegex: &v3matcherpb.RegexMatcher{Regex: "/a/"}},
Headers: []*v3routepb.HeaderMatcher{
{
Name: "th",
HeaderMatchSpecifier: &v3routepb.HeaderMatcher_StringMatch{StringMatch: &v3matcherpb.StringMatcher{MatchPattern: &v3matcherpb.StringMatcher_Exact{Exact: "tv"}}},
},
},
RuntimeFraction: &v3corepb.RuntimeFractionalPercent{
DefaultValue: &v3typepb.FractionalPercent{
Numerator: 1,
Denominator: v3typepb.FractionalPercent_HUNDRED,
},
},
},
Action: &v3routepb.Route_Route{
Route: &v3routepb.RouteAction{
ClusterSpecifier: &v3routepb.RouteAction_WeightedClusters{
WeightedClusters: &v3routepb.WeightedCluster{
Clusters: []*v3routepb.WeightedCluster_ClusterWeight{
{Name: "B", Weight: &wrapperspb.UInt32Value{Value: 60}},
{Name: "A", Weight: &wrapperspb.UInt32Value{Value: 40}},
},
}}}},
},
},
wantRoutes: []*Route{{
Regex: func() *regexp.Regexp { return regexp.MustCompile("/a/") }(),
Headers: []*HeaderMatcher{
{
Name: "th",
InvertMatch: newBoolP(false),
StringMatch: &sm,
},
},
Fraction: newUInt32P(10000),
WeightedClusters: map[string]WeightedCluster{"A": {Weight: 40}, "B": {Weight: 60}},
ActionType: RouteActionRoute,
}},
wantErr: false,
},
{
name: "query is ignored",
routes: []*v3routepb.Route{
Expand Down