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

http: add CDN-Loop header filter #13158

Merged
merged 17 commits into from
Sep 25, 2020
Merged

Conversation

justin-mp
Copy link
Contributor

The CdnLoopFilter implements an HTTP filter that detects and prevents
CDN loops using the RFC 8586 CDN-Loop header. The filter can be
configured with the CDN identifier to look for as well as the number
of times the CDN identifier can be seen before responding with an
error.

Fixes #8183.

Risk Level: Low
Testing: bazel test //test/extensions/filters/http/cdn_loop/...
Docs Changes: N/A
Release Notes: added note about new filter

Signed-off-by: Justin Mazzola Paluska [email protected]

The CdnLoopFilter implements an HTTP filter that detects and prevents
CDN loops using the RFC 8586 CDN-Loop header.  The filter can be
configured with the CDN identifier to look for as well as the number
of times the CDN identifier can be seen before responding with an
error.

Risk Level: Low
Testing: bazel test //test/extensions/filters/http/cdn_loop/...
Docs Changes: N/A
Release Notes: added note about new filter

Signed-off-by: Justin Mazzola Paluska <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13158 was opened by justin-mp.

see: more, trace.

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Signed-off-by: Justin Mazzola Paluska <[email protected]>
Signed-off-by: Justin Mazzola Paluska <[email protected]>
This removes:

checking consistency... /source/generated/rst/api-v3/extensions/filters/http/cdn_loop/v3alpha/cdn_loop.proto.rst: WARNING: document isn't included in any toctree

from the docs CI.

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Looks great - just a few nits

source/extensions/filters/http/cdn_loop/BUILD Show resolved Hide resolved
source/extensions/filters/http/cdn_loop/filter.cc Outdated Show resolved Hide resolved
docs/root/version_history/current.rst Outdated Show resolved Hide resolved
source/extensions/filters/http/cdn_loop/filter.cc Outdated Show resolved Hide resolved
Signed-off-by: Justin Mazzola Paluska <[email protected]>
Signed-off-by: Justin Mazzola Paluska <[email protected]>
Why does the formatter hate two spaces after a period?

Signed-off-by: Justin Mazzola Paluska <[email protected]>
/source/generated/rst/configuration/http/http_filters/cdn_loop.rst:16: WARNING: Unexpected indentation.
/source/generated/rst/configuration/http/http_filters/cdn_loop.rst:13: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/source/generated/rst/configuration/http/http_filters/cdn_loop.rst:28: WARNING: Unexpected indentation.
/source/generated/rst/configuration/http/http_filters/cdn_loop.rst:30: WARNING: Block quote ends without a blank line; unexpected unindent.

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Signed-off-by: Justin Mazzola Paluska <[email protected]>
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Docs look great, thanks!

regarding perf, I think a benchmark with some pathologically complex headers? Basically if the cdn loop header is the largest allowed by Envoy header limits, we want to make sure it's not N^2 expensive. But as said that can all be in a follow-up

source/extensions/filters/http/cdn_loop/filter.cc Outdated Show resolved Hide resolved
Signed-off-by: Justin Mazzola Paluska <[email protected]>
@junr03 junr03 assigned htuch and alyssawilk and unassigned junr03 Sep 22, 2020
This gets us the automatic coalescing we want.

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Looks good modulo two minor test nits!

@snowp would you be up for the non-google pass?

alyssawilk
alyssawilk previously approved these changes Sep 23, 2020
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM regardless of the test param stuff - still prefer to limit but I'm out Thursday and Friday and don't consider it blocking :-)

Signed-off-by: Justin Mazzola Paluska <[email protected]>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks this looks good to me, just one minor comment

test/extensions/filters/http/cdn_loop/config_test.cc Outdated Show resolved Hide resolved
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 api

@htuch htuch removed their assignment Sep 23, 2020
@justin-mp justin-mp requested a review from snowp September 24, 2020 10:53
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Seems like we're blocking on the GCC CI failure for now

@justin-mp
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13158 (comment) was created by @justin-mp.

see: more, trace.

@snowp snowp merged commit c71ec27 into envoyproxy:master Sep 25, 2020
@justin-mp justin-mp deleted the cdn-loop-filter branch October 5, 2020 10:16
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.

Implement CDN loop detection
5 participants