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

Add ProtoApiScrubber HTTP filter configuration proto #38155

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sumitkmr2
Copy link

@sumitkmr2 sumitkmr2 commented Jan 23, 2025

Commit Message: Add ProtoApiScrubber HTTP filter configuration proto.
Additional Description: We are adding a new envoy filter for API filtering for the APIs which are backed by protobuf definitions. This is the first PR for the same which contains just the filter config. Subsequent PRs will include adding more fields to the filter config to support further usecases, filter runtime code, etc. The design doc for this filter is internally approved within Google.
Risk Level: NONE
Testing: NOT DONE. Will be done once the actual filter runtime code is added.
Docs Changes: NOT DONE.
Release Notes: NA. Will be added once the actual filter runtime code is added.
Platform Specific Features: NONE.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Copy link

Hi @sumitkmr2, 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: #38155 was opened by sumitkmr2.

see: more, trace.

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38155 was opened by sumitkmr2.

see: more, trace.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38155 was opened by sumitkmr2.

see: more, trace.

Signed-off-by: Sumit Kumar <[email protected]>
Signed-off-by: Sumit Kumar <[email protected]>
Signed-off-by: Sumit Kumar <[email protected]>
@phlax
Copy link
Member

phlax commented Jan 23, 2025

/docs

Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/38155/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #38155 (comment) was created by @phlax.

see: more, trace.

Signed-off-by: Sumit Kumar <[email protected]>
@sumitkmr2 sumitkmr2 marked this pull request as ready for review January 24, 2025 05:17
Signed-off-by: Sumit Kumar <[email protected]>
Signed-off-by: Sumit Kumar <[email protected]>
@dchakarwarti
Copy link
Contributor

Overall, Looks good to me.

Do you also plan to update the fields in commit message? I think it's safe to mention that the Design Document has been approved internally by the team at Google.

@sumitkmr2
Copy link
Author

Overall, Looks good to me.

Do you also plan to update the fields in commit message? I think it's safe to mention that the Design Document has been approved internally by the team at Google.

Thanks Divya! I have updated the commit message.

option (udpa.annotations.file_status).package_version_status = ACTIVE;
option (xds.annotations.v3.file_status).work_in_progress = true;

// [#not-implemented-hide:]
Copy link
Member

Choose a reason for hiding this comment

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

@sumitkmr2 i would like to review the docs here - i can see issues in the source - but kinda need it rendered

i think because of this its not currently rendering - could you temporarily disable it please

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@phlax phlax self-assigned this Jan 24, 2025
// },
// "restrictions": {
// "method_restrictions": {
// "library.book_service.get_books": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does this method name match the proto?

SOURCE_UNSPECIFIED = 0;

// The entitlement will be fetched from the request header ``key``.
REQUEST_HEADER = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this header come from client?

Source source = 1;

// The key to lookup the entitlement from the specified ``source``.
string key = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this restriction type?

@phlax
Copy link
Member

phlax commented Jan 24, 2025

docs build is failing with

Did you forget to add 'envoy.filters.http.proto_api_scrubber' to extensions_build_config.bzl, extensions_metadata.yaml, contrib_build_config.bzl, or contrib/extensions_metadata.yaml?

i think it needs to be setup correctly - wondering if there is an implementation - its not so common to add just an api without implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants