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

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

Merged
merged 2 commits into from May 25, 2023

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented May 24, 2023

Fixes #6226. This PR adds support for String Matching in Header Matching for routes. It converts and validates the proto StringMatcher received from the control plane in the xDS Client, then creates a HeaderMatcher for it in ServiceConfig. Tests at xDS Client layer and header matcher layer, anything further will be too specific for an e2e test (as discussed about testing Philosophy with Eric and Doug, leave more specific behavior checks to unit tests and e2e tests as sanity checks to not bloat number and time of e2e tests).

RELEASE NOTES:

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

@zasweq zasweq requested a review from dfawley May 24, 2023 12:02
@zasweq zasweq added this to the 1.56 Release milestone May 24, 2023
if !ok {
return false
}
return hsm.stringMatcher.Match(v) != hsm.invert
Copy link
Contributor

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

md: metadata.Pairs("th", "not-match"),
invert: true,
want: true,
},
Copy link
Contributor

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.

Comment on lines 476 to 477
md metadata.MD
invert bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: please swap these two in order (here and in the test cases) since invert is part of the matcher and md is the input passed to the matcher. OR: build the matcher in the testcase list and put it in there instead of the individual fields.

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. Chose the first option and swapped.

@zasweq zasweq assigned dfawley and unassigned zasweq May 25, 2023
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks!

@dfawley dfawley assigned zasweq and unassigned dfawley May 25, 2023
@zasweq zasweq merged commit 4d3f221 into grpc:master May 25, 2023
11 checks passed
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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for String Matcher in Header Matching
2 participants