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

tap: introduce HTTP tap filter #5515

Merged
merged 44 commits into from
Jan 21, 2019
Merged

tap: introduce HTTP tap filter #5515

merged 44 commits into from
Jan 21, 2019

Conversation

mattklein123
Copy link
Member

This is a MVP for the HTTP tap filter. It includes minimal
infrastructure for the following:

  1. Generic tap configuration which in the future will be used for
    static config, XDS config, etc. In this MVP the tap can be
    configured via a /tap admin endpoint.
  2. Generic output configuration which in the future will be used for
    different output sinks such as files, gRPC API, etc. In this MVP
    the tap results are streamed back out the /tap admin endpoint.
  3. Matching infrastructure. In this MVP only matching on request and
    response headers are implemented. Both logical AND and logical OR
    matches are possible.
  4. In this MVP request/response body is not considered at all.
  5. All docs are included and with all the caveats the filter is ready
    to use for the limited cases it supports (which are likely still to
    be useful).

There is a lot of follow on work which I will do in subsequent PRs.
This includes:

  1. Merging the existing capture transport socket into this framework.
  2. Implementing body support, both for matching on body contents as
    well as outputting body data.
  3. Tap rate limiting so too many streams do not get tapped.
  4. gRPC matching. Using reflection and loaded proto definitions, it will
    be possible to match on gRPC fields.
  5. JSON matching. If the body parses as JSON, we can allow matching on
    JSON fields.

Part of #1413.

Risk Level: Low. New feature with no dependencies. Minor changes to a few other files.
Testing: Unit/integration tests.
Docs Changes: Added.
Release Notes: Added.

This is a MVP for the HTTP tap filter. It includes minimal
infrastructure for the following:
1. Generic tap configuration which in the future will be used for
   static config, XDS config, etc. In this MVP the tap can be
   configured via a /tap admin endpoint.
2. Generic output configuration which in the future will be used for
   different output sinks such as files, gRPC API, etc. In this MVP
   the tap results are streamed back out the /tap admin endpoint.
3. Matching infrastructure. In this MVP only matching on request and
   response headers are implemented. Both logical AND and logical OR
   matches are possible.
4. In this MVP request/response body is not considered at all.
5. All docs are included and with all the caveats the filter is ready
   to use for the limited cases it supports (which are likely still to
   be useful).

There is a lot of follow on work which I will do in subsequent PRs.
This includes:
1. Merging the existing capture transport socket into this framework.
2. Implementing body support, both for matching on body contents as
   well as outputting body data.
3. Tap rate limiting so too many streams do not get tapped.
4. gRPC matching. Using reflection and loaded proto definitions, it will
   be possible to match on gRPC fields.
5. JSON matching. If the body parses as JSON, we can allow matching on
   JSON fields.

Part of #1413.

Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
@mattklein123
Copy link
Member Author

@dio @lizan since this feature is of general interest to the Envoy/Istio community can one/both of you potentially own review of it? @htuch also in case you are interested. My plan is to refactor the transport socket capture feature to work within this framework next.

@lizan lizan self-requested a review January 7, 2019 18:33
api/envoy/data/tap/v2alpha/http.proto Outdated Show resolved Hide resolved
source/extensions/common/tap/BUILD Show resolved Hide resolved
source/extensions/filters/http/tap/tap_config_impl.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/tap/tap_config_impl.cc Outdated Show resolved Hide resolved
source/extensions/filters/http/tap/tap_config_impl.cc Outdated Show resolved Hide resolved
source/extensions/common/tap/admin.cc Outdated Show resolved Hide resolved
source/extensions/common/tap/admin.cc Outdated Show resolved Hide resolved
@lizan lizan self-assigned this Jan 8, 2019
@lizan
Copy link
Member

lizan commented Jan 8, 2019

First pass, didn't take detailed look at tests yet.

@mattklein123
Copy link
Member Author

Replicating a comment from Slack:

@alyssawilk @zuercher @lizan With Lizan's help getting a log, I'm realizing that I don't think I can get the tap integration test working on OSX. The issue comes down to #4294. Basically, the admin /tap response is a streamed response that never ends, and the test relies on the admin client being able to hang up when it's done getting data, but on OSX Envoy never sees the hangup and cleans up the admin client. Any thoughts here on what I can do? I can't think of any way to fix this so I'm wondering if I shouldn't run the test on OSX? Ideas? https://github.com/envoyproxy/envoy/pull/5515/files#diff-975313274dec5c7ffca88fb1be76d717R95

@mattklein123
Copy link
Member Author

To follow up here, @lizan had the very good idea to have the test use HTTP/2 vs. HTTP/1 which will work. Unfortunately, admin is hard coded to use HTTP/1 currently, so I will make admin do auto codec detection. I will end up splitting all the admin changes out into a separate PR once I have this passing tests.

Signed-off-by: Matt Klein <[email protected]>
@mattklein123
Copy link
Member Author

@htuch @lizan I bit the bullet and rewrote the matching system to be a lot more generic and include and/or/any/not, etc. In doing so, I realized that the matching we need to do to support streaming matching (compute a match as data goes by vs. have everything available) is a lot more complicated than what we do for other matching in Envoy, and I'm skeptical we can share everything at a high level. I do think we can share the low-level matchers (headers, subnet, etc.) and I will work on that.

Anyway, PTAL. I added a bunch of comments on how the matchers work with regard to streaming. Even though this MVP does not support stream tapping today, we are setup to do so, and it's something that I know we will need to support shortly.

Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

I like the match design, hoping we can make this more generic and shared over time. I'll take another pass in the next day with a finer granularity, but it basically looks good to go.

api/envoy/service/tap/v2alpha/common.proto Outdated Show resolved Hide resolved
api/envoy/service/tap/v2alpha/common.proto Outdated Show resolved Hide resolved
api/envoy/service/tap/v2alpha/common.proto Outdated Show resolved Hide resolved
source/extensions/common/tap/admin.cc Outdated Show resolved Hide resolved
@mattklein123
Copy link
Member Author

@htuch @lizan updated per comments

lizan
lizan previously approved these changes Jan 17, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Cool. I'm good with the entire PR modulo what we're doing with matchers/streams, where I have some open questions.

source/extensions/common/tap/tap.h Outdated Show resolved Hide resolved
source/extensions/common/tap/tap.h Outdated Show resolved Hide resolved
}

// Per above, move the matcher into its position.
matchers[new_matcher->index()] = std::move(new_matcher);
Copy link
Member

Choose a reason for hiding this comment

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

How come we need the vector inlining? Can't we just build a regular tree and maintain a pointer to the current node of interest, tracking the leaf-parent path booleans as a vector if necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't follow what you are suggesting. This implementation allows a per-request status vector to be created and then trivially updated without worrying about pointers. IMO it's both the simplest and most efficient implementation for what we need to do.

Copy link
Member

@htuch htuch Jan 20, 2019

Choose a reason for hiding this comment

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

Fair enough, I don't disagree that it's an efficient implementation. I'm visualizing this as at any point in time, for a given request, you have the match tree (immutable and shared) and your current status, which is those parts that have matched. I can see the attractiveness of the flattening due to the fact that you can easily map either way between match predicate and status.

@mattklein123
Copy link
Member Author

@htuch updated PTAL

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, fine to address comment in later PR.

envoy::service::tap::v2alpha::MatchPredicate config_;
};

TEST_F(TapMatcherTest, Any) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have more complete tests here? Happy to punt this to a later PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should have targeted unit tests here. All of the other matchers have coverage via the integration tests, but will definitely add more UTs here in future PRs.

@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: cannot load github.com/repokitteh/modules/lib/circleci.star: load("github.com/repokitteh/modules/lib/circleci.star") error: module load error
🐱

Caused by: a #5515 (review) was submitted by @htuch.

see: more, trace.

@mattklein123 mattklein123 merged commit cf80045 into master Jan 21, 2019
@mattklein123 mattklein123 deleted the tap_mvp branch January 21, 2019 01:39
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 24, 2019
This is a MVP for the HTTP tap filter. It includes minimal
infrastructure for the following:
1. Generic tap configuration which in the future will be used for
   static config, XDS config, etc. In this MVP the tap can be
   configured via a /tap admin endpoint.
2. Generic output configuration which in the future will be used for
   different output sinks such as files, gRPC API, etc. In this MVP
   the tap results are streamed back out the /tap admin endpoint.
3. Matching infrastructure. In this MVP only matching on request and
   response headers are implemented. Both logical AND and logical OR
   matches are possible.
4. In this MVP request/response body is not considered at all.
5. All docs are included and with all the caveats the filter is ready
   to use for the limited cases it supports (which are likely still to
   be useful).

There is a lot of follow on work which I will do in subsequent PRs.
This includes:
1. Merging the existing capture transport socket into this framework.
2. Implementing body support, both for matching on body contents as
   well as outputting body data.
3. Tap rate limiting so too many streams do not get tapped.
4. gRPC matching. Using reflection and loaded proto definitions, it will
   be possible to match on gRPC fields.
5. JSON matching. If the body parses as JSON, we can allow matching on
   JSON fields.

Part of envoyproxy#1413.

Signed-off-by: Matt Klein <[email protected]>

Signed-off-by: Dan Zhang <[email protected]>
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
These are various changes split out from
envoyproxy#5515. They include:

1) Allow the admin server to be accessed over HTTP/2 with prior
   knowledge. This is required to get the tap integration tests to
   pass on OSX.
2) Change admin to buffer the complete request so that admin handlers
   can support POST bodies.
3) Small integration test fix in waitForBodyData() to avoid a race
   condition in which the body data arrives before the call.
4) Small comment and clang-tidy fixes.

Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
This is a MVP for the HTTP tap filter. It includes minimal
infrastructure for the following:
1. Generic tap configuration which in the future will be used for
   static config, XDS config, etc. In this MVP the tap can be
   configured via a /tap admin endpoint.
2. Generic output configuration which in the future will be used for
   different output sinks such as files, gRPC API, etc. In this MVP
   the tap results are streamed back out the /tap admin endpoint.
3. Matching infrastructure. In this MVP only matching on request and
   response headers are implemented. Both logical AND and logical OR
   matches are possible.
4. In this MVP request/response body is not considered at all.
5. All docs are included and with all the caveats the filter is ready
   to use for the limited cases it supports (which are likely still to
   be useful).

There is a lot of follow on work which I will do in subsequent PRs.
This includes:
1. Merging the existing capture transport socket into this framework.
2. Implementing body support, both for matching on body contents as
   well as outputting body data.
3. Tap rate limiting so too many streams do not get tapped.
4. gRPC matching. Using reflection and loaded proto definitions, it will
   be possible to match on gRPC fields.
5. JSON matching. If the body parses as JSON, we can allow matching on
   JSON fields.

Part of envoyproxy#1413.

Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants