Skip to content

Add SXG filter extension#17215

Merged
mattklein123 merged 68 commits intoenvoyproxy:mainfrom
cpapazian:sxg_filter_2
Sep 2, 2021
Merged

Add SXG filter extension#17215
mattklein123 merged 68 commits intoenvoyproxy:mainfrom
cpapazian:sxg_filter_2

Conversation

@cpapazian
Copy link
Copy Markdown

Commit Message: Add SXG filter extension
Additional Description: Filter to transform response to Signed HTTP Exchange (SXG) package using libsxg (https://github.com/google/libsxg). See: https://wicg.github.io/webpackage/draft-yasskin-httpbis-origin-signed-exchanges-impl.html
Risk Level: low, new filter
Testing: unit tests included, filter is based off version running in production for 4 months
Docs Changes: filter docs added
Release Notes: release notes included

Discussed previously in #9763

@repokitteh-read-only
Copy link
Copy Markdown

Hi @cpapazian, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17215 was opened by cpapazian.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Jul 1, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #17215 was opened by cpapazian.

see: more, trace.

@cpapazian
Copy link
Copy Markdown
Author

@rgs1 @twifkak

@moderation
Copy link
Copy Markdown
Contributor

Introduces a new dependency - https://github.com/envoyproxy/envoy/blob/main/DEPENDENCY_POLICY.md#new-external-dependencies. OSSF Scorecard results:

scorecard --repo=https://github.com/google/libsxg
RESULTS
Active: Pass 10
Automatic-Dependency-Update: Fail 3
Binary-Artifacts: Pass 10
Branch-Protection: Fail 0
CI-Tests: Pass 9
CII-Best-Practices: Fail 10
Code-Review: Pass 10
Contributors: Pass 10
Frozen-Deps: Fail 10
Fuzzing: Fail 10
Packaging: Fail 10
Pull-Requests: Pass 9
SAST: Fail 10
Security-Policy: Fail 5
Signed-Releases: Fail 10
Signed-Tags: Fail 10
Token-Permissions: Fail 10
Vulnerabilities: Pass 10

CODEOWNERS Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Anyone from @envoyproxy/maintainers to sponsor?

@cpapazian cpapazian force-pushed the sxg_filter_2 branch 3 times, most recently from efe689d to a62798f Compare July 4, 2021 17:25
@cpapazian
Copy link
Copy Markdown
Author

@twifkak is it possible to get libsxg to build with cmake 3.10? the arm build is running cmake 3.10.2 currently, and fails:

-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:67 (cmake_minimum_required):
  CMake 3.13 or higher is required.  You are running version 3.10.2

https://dev.azure.com/cncf/envoy/_build/results?buildId=81398&view=logs&j=767be981-567e-57d8-68c3-2140ede0a0bd&t=1fb0b72f-6293-562d-a4d1-bb649223eaca&l=15025

running with the previous commit of libsxg (before we added the 3.13 requirement) we see:

CMake Error at CMakeLists.txt:161 (install):
  install TARGETS given no ARCHIVE DESTINATION for static library target
  "sxg_static".

https://dev.azure.com/cncf/envoy/_build/results?buildId=81199&view=logs&j=767be981-567e-57d8-68c3-2140ede0a0bd&t=1fb0b72f-6293-562d-a4d1-bb649223eaca&l=420

not really sure what to make of this. would be curious your thoughts ...

@twifkak
Copy link
Copy Markdown

twifkak commented Jul 8, 2021

@cpapazian Yep, I'll send a PR to update the minimum requirement to 3.1 (earliest I could test easily on my x86_64 laptop via https://cmake.org/files/) when RUN_TEST is off.

It looks like the subsequent error can be fixed by adding the flags -DCMAKE_INSTALL_LIBDIR=lib -DCMAKE_INSTALL_BINDIR=bin. I guess those became the default in some cmake 3.11+ version? Anyway, I'll just include those flags in the README of the same PR.

@twifkak
Copy link
Copy Markdown

twifkak commented Jul 8, 2021

@cpapazian Merged google/libsxg#95 which hopefully fixes this (with tests to prevent regression).

Chris Papazian added 14 commits July 13, 2021 20:11
Filter to transform response to Signed HTTP Exchange (SXG) package using
libsxg (https://github.com/google/libsxg).

Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
per @rgs1 suggestion, add @alyssawilk to codeowners so that we can pass tests.

Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Chris Papazian added 2 commits July 30, 2021 00:36
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Chris Papazian added 7 commits July 30, 2021 22:08
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
@cpapazian
Copy link
Copy Markdown
Author

@yanavlasov I think this is ready for review. Let me know if there's anything else you need from me first. Thanks!

cc @rgs1

@cpapazian
Copy link
Copy Markdown
Author

per @rgs1, we now have a contrib directory (#17595). I can move this extension there, if that makes sense. let me know which you prefer, @yanavlasov

@junr03
Copy link
Copy Markdown
Member

junr03 commented Aug 30, 2021

@yanavlasov friendly ping on this.

Chris Papazian added 5 commits August 30, 2021 22:12
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
Signed-off-by: Chris Papazian <papazian@pinterest.com>
@cpapazian
Copy link
Copy Markdown
Author

@yanavlasov - per offline conversation with @rgs1, i went ahead and moved this over to contrib.

@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Sep 1, 2021

@mattklein123 does this sound like a reasonable candidate for contrib/? We were initially targeting it as a regular filter, but contrib/ would be great for us so we can share the code with other parties and then possibly graduate out of contrib... Thanks!

@mattklein123
Copy link
Copy Markdown
Member

@mattklein123 does this sound like a reasonable candidate for contrib/? We were initially targeting it as a regular filter, but contrib/ would be great for us so we can share the code with other parties and then possibly graduate out of contrib... Thanks!

Sure! Is this ready to review? I can take a look and merge if so.

@cpapazian
Copy link
Copy Markdown
Author

Sure! Is this ready to review? I can take a look and merge if so.

Yeah, I think we're ready. @rgs1 has already taken a pass at review.

Only question we had is what to do with the libsxg dependency now that the filter has moved to contrib. It wasn't obvious to me how to dis-entangle all of the external repo stuff in the builds, and I'm not sure we even want to.

Thanks!

@mattklein123 mattklein123 self-assigned this Sep 1, 2021
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks I'm fine with this going in for contrib for now, with a few small doc comments.

Regarding the build dependencies being in core, in a perfect world we would figure out how to have them live with the contrib extension itself but I don't think we have a good pattern for that so I'm OK with this for now, especially since bazel will only build the dependency if it's actually used.

/wait

* listener: new listener metric ``downstream_cx_transport_socket_connect_timeout`` to track transport socket timeouts.
* rbac: added :ref:`destination_port_range <envoy_v3_api_field_config.rbac.v3.Permission.destination_port_range>` for matching range of destination ports.
* route config: added :ref:`dynamic_metadata <envoy_v3_api_field_config.route.v3.RouteMatch.dynamic_metadata>` for routing based on dynamic metadata.
* sxg_filter: added filter to transform response to SXG package. This can be enabled by setting :ref:`SXG <envoy_v3_api_msg_extensions.filters.http.sxg.v3alpha.SXG>` configuration.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

note this is available in contrib builds only with a ref link to the contrib docs.

Comment on lines +7 to +8
* :ref:`v3 API reference <envoy_v3_api_msg_extensions.filters.http.sxg.v3alpha.SXG>`
* This filter should be configured with the name *envoy.filters.http.sxg*.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you note this is in contrib builds only with a ref link to the contrib docs. If the squash filter docs don't have that can you fix that also? (I know I automated the other docs but I think for these manual docs it is probably missing.)

Signed-off-by: Chris Papazian <papazian@pinterest.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 4aab7fa into envoyproxy:main Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants