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 grpc_httpjson_transcoder external dependency to core #15304

Closed
qiwzhang opened this issue Mar 4, 2021 · 16 comments
Closed

Add grpc_httpjson_transcoder external dependency to core #15304

qiwzhang opened this issue Mar 4, 2021 · 16 comments
Labels
deps Approval required for changes to Envoy's external dependencies

Comments

@qiwzhang
Copy link
Contributor

qiwzhang commented Mar 4, 2021

This is for #15299 to address #7763

Title: Add grpc_httpjson_transcoder external dependency to core

Description:
grpc_httpjson_transcoding external dependency is already used by http filter extension grpc_transcoder.

In order to support url_template in RouteMatch, propose to add this dependency to core:

Following items (from check list) may have issues:

  • SecPolicy: no
  • CI-Tests: no
  • Releases: no
@qiwzhang qiwzhang added the triage Issue requires triage label Mar 4, 2021
@qiwzhang qiwzhang changed the title Add a new external dependency to core Add grpc_httpjson_transcoder external dependency to core Mar 4, 2021
@qiwzhang
Copy link
Contributor Author

qiwzhang commented Mar 4, 2021

@envoyproxy/dependency-shepherds @envoyproxy/security-team

@dio
Copy link
Member

dio commented Mar 4, 2021

@qiwzhang do you accept external contributions for enabling CI in that repo? :).

@dio dio added deps Approval required for changes to Envoy's external dependencies and removed triage Issue requires triage labels Mar 4, 2021
@qiwzhang
Copy link
Contributor Author

qiwzhang commented Mar 4, 2021

Yes @dio

@qiwzhang
Copy link
Contributor Author

qiwzhang commented Mar 4, 2021

Hi @nareddyt could you help to add CI test for grpc_httpjson_transcoding repo?

@nareddyt
Copy link
Contributor

nareddyt commented Mar 4, 2021

I still do not have admin access to the repo. I will try again.

@moderation
Copy link
Contributor

moderation commented Mar 5, 2021

Full results from OSSF Scorecard is:

scorecard --repo=https://github.com/grpc-ecosystem/grpc-httpjson-transcoding

RESULTS
-------
Active: Pass 10
Branch-Protection: Fail 0
CI-Tests: Fail 4
CII-Best-Practices: Fail 10
Code-Review: Pass 10
Contributors: Pass 10
Frozen-Deps: Fail 5
Fuzzing: Fail 10
Packaging: Fail 0
Pull-Requests: Pass 10
SAST: Fail 10
Security-Policy: Fail 0
Signed-Releases: Fail 0
Signed-Tags: Fail 0

Descriptions of categories at https://github.com/ossf/scorecard#checks. In addition to adding a security policy and CI, doing code scanning with CodeQL might be another quick win. Some of the dependencies in repositories.bzl are themselves old - protobuf 3.10.1 as an example.

@nareddyt
Copy link
Contributor

nareddyt commented Mar 5, 2021

@qiwzhang I was planning on updating the dep for protobuf tomorrow. I can handle CI setup as well once I have permissions.

I don't have background on the other items. @moderation which ones are hard blockers?

@moderation
Copy link
Contributor

moderation commented Mar 5, 2021

@nareddyt CI and Security Policy are both MUST for core deps - https://github.com/envoyproxy/envoy/blob/main/DEPENDENCY_POLICY.md#new-external-dependencies (and I've added links to my comment above)

@qiwzhang
Copy link
Contributor Author

@moderation

  1. CI is added
  2. security policy is added.

Could you check again?

Thanks

@moderation
Copy link
Contributor

scorecard is picking up your security policy. Not sure why your CI score is unchanged.

RESULTS                    
-------                    
Active: Pass 10            
Branch-Protection: Fail 0  
CI-Tests: Fail 4           
CII-Best-Practices: Fail 10
Code-Review: Pass 10       
Contributors: Pass 10      
Frozen-Deps: Fail 5        
Fuzzing: Fail 10           
Packaging: Fail 0          
Pull-Requests: Pass 10     
SAST: Fail 10              
Security-Policy: Pass 10   
Signed-Releases: Fail 0    
Signed-Tags: Fail 0        

@envoyproxy/dependency-shepherds for review

@antoniovicente
Copy link
Contributor

Is it worth considering splitting the url_template code into a separate library that can be consumed by core?

Do we have benchmarks that compare the url_template implementation from grpc_httpjson_transcoder to equivalent RE2 regexps that match the same inputs?

@qiwzhang
Copy link
Contributor Author

It is a good idea to split path_matcher code used for url_template match from grpc_httpjson_transcoder. I will explore that.

@qiwzhang
Copy link
Contributor Author

For performance, @nareddyt has a performance test. Maybe he can add url_template perf test with the pending pr to get the number

@nareddyt
Copy link
Contributor

For performance, @nareddyt has a performance test. Maybe he can add url_template perf test with the pending pr to get the number

Sure, I'll get started on that today.

@nareddyt
Copy link
Contributor

@antoniovicente @qiwzhang the benchmark is added in #15493. Fairly interesting - I expected it to be slow, but not slower than the equivalent regex route matchers. I guess building the whole tree structure but not taking advantage of combining multiple routes adds a lot of overhead.

@qiwzhang
Copy link
Contributor Author

This issue can be replaced by #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

No branches or pull requests

5 participants