http: add CDN-Loop header value parser and utilities by justin-mp · Pull Request #12894 · envoyproxy/envoy

justin-mp · 2020-08-31T17:17:46Z

The CDN-Loop header is defined by RFC 8586 and is a comma-separated
list of values. However, it’s not possible to separate the values by
splitting on ',' because the values may have parameters, which
themselves may contain strings, which may contain commas. Thus, we
need a real parser to extract the values.

This pull request contains a CDN-Loop header parser and a utility that
uses the parser to count how many times a particular CDN’s identifier
has been seen in the header.

The parser and the utility are the first two steps towards having a
CDN-Loop aware filter. See #8183.

Risk: Low
Testing: bazel test //test/extensions/filters/http/cdn:cdn_loop_parser_test //test/extensions/filters/http/cdn:cdn_loop_utils_test
Documentation: N/A
Release Notes: N/A

Signed-off-by: Justin Mazzola Paluska justinmp@google.com

The CDN-Loop header is defined by RFC 8586 and is a comma-separated list of values. However, it’s not possible to separate the values by splitting on `','` because the values may have parameters, which themselves may contain strings, which may contain commas. Thus, we need a real parser to extract the values. This pull request contains a CDN-Loop header parser and a utility that uses the parser to count how many times a particular CDN’s identifier has been seen in the header. The parser and the utility are the first two steps towards having a CDN-Loop aware filter. See envoyproxy#8183. Risk: Low Testing: bazel test //test/extensions/filters/http/cdn:cdn_loop_parser_test //test/extensions/filters/http/cdn:cdn_loop_utils_test Documentation: N/A Release Notes: N/A Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

This fixes the following style issues: ERROR: From ./test/extensions/filters/http/cdn/cdn_loop_parser_test.cc ERROR: ./test/extensions/filters/http/cdn/cdn_loop_parser_test.cc:140: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: ./test/extensions/filters/http/cdn/cdn_loop_parser_test.cc:144: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: ./test/extensions/filters/http/cdn/cdn_loop_parser_test.cc:213: Don't use designated initializers in struct initialization, they are not part of C++14 Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

The fix_format tool complains: ERROR: From ./source/extensions/filters/http/cdn/cdn_loop_parser.h ERROR: ./source/extensions/filters/http/cdn/cdn_loop_parser.h:87: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: ./source/extensions/filters/http/cdn/cdn_loop_parser.h:114: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: ./source/extensions/filters/http/cdn/cdn_loop_parser.h:137: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: ./source/extensions/filters/http/cdn/cdn_loop_parser.h:159: Don't use designated initializers in struct initialization, they are not part of C++14 ERROR: check format failed. run 'tools/code_format/check_format.py fix' even though this isn’t a designated initializer, just a string that looks like one. Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp · 2020-08-31T18:21:15Z

@alyssawilk : I added you as a code owner in addition to @penguingao and me. Please let me know if there's a better maintainer to add as a code owner.

alyssawilk

Thanks for putting this together - I'm happy to co-own :-)

I'll let Peng do first review pass, and just note a few things in passing.

source/extensions/filters/http/cdn/cdn_loop_parser.cc

source/extensions/filters/http/cdn/cdn_loop_parser.h

source/extensions/filters/http/cdn/cdn_loop_utils.cc

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

penguingao

Flushing some first round comments out - I have not looked at the test. But this change seems to deserve a fuzz test. @asraa

source/extensions/filters/http/cdn/cdn_loop_parser.cc

source/extensions/filters/http/cdn/cdn_loop_parser.h

source/extensions/filters/http/cdn/cdn_loop_parser.cc

test/extensions/filters/http/cdn/status_helpers.h

source/extensions/filters/http/cdn/cdn_loop_parser.h

source/extensions/filters/http/cdn/cdn_loop_parser.cc

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

asraa

Parsing code would be great to add fuzzers for! It seems like you'll be able to make relatively simple fuzzers for this. Feel free to make a github issue or slack dm where we can chat about the design if you need any help! I haven't taken too close of a look on the code yet, but can do soon.

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

The filter name will get embedded into various protos, so having the more accurate name of “cdn_loop” is better. Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

This also makes the names match other extensions, like the cache one. Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp · 2020-09-10T20:41:15Z

One overall thing to look at is it's a bit below the Envoy coverage bar of 96.6 (Once per-extension coverage checks are fixedby #13030 and this won't merge.)

I increased test coverage to 100% according to that link. (You may have to shift+reload to get the newest figures.)

justin-mp · 2020-09-11T16:42:49Z

I think I've addressed all of the comments at this point. I just merged from master to get the latest changes. Let me know if there's anything more to do!

CODEOWNERS

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp · 2020-09-16T20:47:26Z

@lizan Friendly ping! Anything more you need me to do?

alyssawilk

Lizan's out and his comments are addressed, so I'm going to go ahead and merge and we can sort out any further comments in a follow-up when Lizan is back.

justin-mp added 4 commits August 31, 2020 14:06

Fix up style issues

308efac

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Add an ownership directory for http/cdn

734288b

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp force-pushed the cdn-loop-parser branch from 79e5fc9 to 734288b Compare August 31, 2020 18:07

justin-mp added 2 commits August 31, 2020 14:12

Add alyssawilk as code owner for now

5d1f5f5

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

alyssawilk reviewed Aug 31, 2020

View reviewed changes

source/extensions/filters/http/cdn/cdn_loop_parser.cc Outdated Show resolved Hide resolved

source/extensions/filters/http/cdn/cdn_loop_parser.h Show resolved Hide resolved

source/extensions/filters/http/cdn/cdn_loop_utils.cc Show resolved Hide resolved

alyssawilk assigned penguingao and alyssawilk Aug 31, 2020

justin-mp added 2 commits August 31, 2020 15:56

Teach the spell checker about CDNs

40b62fb

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Teach the spell checker about more RFC terms

786c47d

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp force-pushed the cdn-loop-parser branch from f9862ac to 786c47d Compare September 1, 2020 15:07

justin-mp added 2 commits September 1, 2020 15:24

clang-tidy fixes

2ee7ced

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Merge branch 'master' into cdn-loop-parser

aef2b60

penguingao reviewed Sep 2, 2020

View reviewed changes

justin-mp added 2 commits September 2, 2020 11:34

Rename bazel targets to match Envoy naming convention

72d7de4

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

First pass at @penguingao’s comments

874ca27

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp force-pushed the cdn-loop-parser branch from 341d92d to 874ca27 Compare September 2, 2020 15:34

justin-mp added 2 commits September 2, 2020 11:40

Add better token error suggested by @penguingao

155d65a

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Clean up parseCdnId as suggested by @penguingao

61f65b3

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

asraa reviewed Sep 2, 2020

View reviewed changes

justin-mp added 7 commits September 2, 2020 13:48

Move StatusHelpers to test/test_common

2712cc7

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Rename update as per comments from @penguingao

41d0df7

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Expand comment on why parsing by token is OK.

a48d4a2

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Rename the cdn directory to cdn_loop

8574436

The filter name will get embedded into various protos, so having the more accurate name of “cdn_loop” is better. Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Make file names less stuttery

c53f7e8

This also makes the names match other extensions, like the cache one. Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Clean up namespaces

c81acbf

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Merge branch 'master' into cdn-loop-parser

a5627cb

justin-mp dismissed penguingao’s stale review via 0dc94b3 September 10, 2020 14:50

justin-mp added 12 commits September 10, 2020 11:32

Transposed numbers police

6caa904

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Comment police

9ad718d

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Rename parseOptionalWhitespace to skipOptionalWhitespace

89c0ab3

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Optimize IPv6 and quoted string paths

be8d21a

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Fix copy pasta from test

bdde63d

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Wrong word police

410b78e

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage for parseQuotedString

c734c91

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage for parseCdnId

d93ba73

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage for parseParameter

429f4e5

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage for parseCdnInfoList

a9a2c77

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage for countCdnLoopOccurrences

b6b3764

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Increase test coverage of parser.h

2e95d94

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp force-pushed the cdn-loop-parser branch from 81f95ea to 2e95d94 Compare September 10, 2020 19:43

Make error messages consistent.

4428c59

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

Merge branch 'master' into cdn-loop-parser

3145556

alyssawilk previously approved these changes Sep 14, 2020

View reviewed changes

lizan reviewed Sep 14, 2020

View reviewed changes

CODEOWNERS Outdated Show resolved Hide resolved

Missed rename of cdn to cdn_loop

e3eae40

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

justin-mp dismissed alyssawilk’s stale review via e3eae40 September 14, 2020 18:01

Fix style failure (std::string_view versus absl::string_view)

a47b097

Signed-off-by: Justin Mazzola Paluska <justinmp@google.com>

alyssawilk assigned lizan Sep 15, 2020

alyssawilk approved these changes Sep 17, 2020

View reviewed changes

alyssawilk merged commit 9eeba8f into envoyproxy:master Sep 17, 2020

justin-mp deleted the cdn-loop-parser branch September 17, 2020 20:09

justin-mp mentioned this pull request Sep 18, 2020

Fuzz test the CDN-Loop header parser #13179

Closed

Conversation

justin-mp commented Aug 31, 2020

Uh oh!

justin-mp commented Aug 31, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penguingao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

justin-mp commented Sep 10, 2020

Uh oh!

justin-mp commented Sep 11, 2020

Uh oh!

Uh oh!

justin-mp commented Sep 16, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants