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

feat: grpc-web plugin supports trailers as part of response body #10613

Closed
wants to merge 10 commits into from

Conversation

Affanmir
Copy link

@Affanmir Affanmir commented Dec 7, 2023

Description

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • [] I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@Affanmir
Copy link
Author

Affanmir commented Dec 7, 2023

@shreemaan-abhishek #10573 Linked Issue

@Affanmir Affanmir changed the title GRPC-web now supports trailers as part of response body feat: GRPC-web now supports trailers as part of response body Dec 7, 2023
@Affanmir Affanmir changed the title feat: GRPC-web now supports trailers as part of response body feat: GRPC-web supports trailers as part of response body Dec 7, 2023
@Affanmir Affanmir changed the title feat: GRPC-web supports trailers as part of response body feat: grpc-web plugin supports trailers as part of response body Dec 7, 2023
@shreemaan-abhishek
Copy link
Contributor

@Affanmir great job, can you add test cases as well?

@Affanmir
Copy link
Author

Affanmir commented Dec 7, 2023

@Affanmir great job, can you add test cases as well?

Sure I'll have to have a look on what framework you are using

@shreemaan-abhishek
Copy link
Contributor

@Affanmir
Copy link
Author

Affanmir commented Dec 8, 2023

This needs some more work, I want to introduce a few more "Access-Control-Allow-Headers" to support different web apps. I'll also want to fix another small issue related to prefix matching paths for routes.

@monkeyDluffy6017
Copy link
Contributor

@Affanmir is this pr ready?

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Dec 11, 2023
@Vacant2333
Copy link
Contributor

@Affanmir hello~ whats the status of the pr now, will it be process~

@Affanmir
Copy link
Author

Affanmir commented Jan 1, 2024

@Affanmir hello~ whats the status of the pr now, will it be process~

ready for review and merge

@shreemaan-abhishek
Copy link
Contributor

@Affanmir the CI is failing.

@Affanmir
Copy link
Author

Affanmir commented Jan 8, 2024

@shreemaan-abhishek can the workflows be rerun? apologies for the delay on my end

@sheharyaar
Copy link
Contributor

@Affanmir , can you please list down the tasks that are complete with this PR ?
We can also proceed like this :

  1. If you wish to work on the complete PR that is also great
  2. If you feel it would take much time, then you can handover the PR and I can continue on this. For this I will need the list of subtasks (completed and remaining ones). Since this is a high priority bug, we would like to fix this fast.

So what do you say ?

@Affanmir
Copy link
Author

So what do you say ?

The PR is complete.

What this PR does

  1. Fix missing trailers issue due to inconsistent grpc implementation
  2. Fix prefix matched by path pattern in the path used for patterns with the format ./

Further bugs/features missing in this plugin that you can work on

  1. Cors plugin does not work with this plugin
  2. GRPC Response codes conversion need to be added

@sheharyaar
Copy link
Contributor

@shreemaan-abhishek , please run the workflow.

@sheharyaar
Copy link
Contributor

@Affanmir apart from the test-case you created, there should be a test for the grpc-status trailer, as it was encountered in your issue

@sheharyaar
Copy link
Contributor

@Affanmir, please run make lint and correct the lint issues.

@sheharyaar
Copy link
Contributor

CORS plugin does not work with this plugin

@Affanmir , are you referring to this :

If a request is made for a resource on another origin which returns the CORs headers, then the type is cors. cors and basic responses are almost identical except that a cors response restricts the headers you can view to Cache-Control, Content-Language, Content-Type, Expires, Last-Modified, and Pragma.

Ref : https://web.dev/articles/introduction-to-fetch#response_types

@Affanmir
Copy link
Author

@Affanmir apart from the test-case you created, there should be a test for the grpc-status trailer, as it was encountered in your issue

Not possible as that would require using a different testing language go's implementation supports this natively, will have to create a java backend.

@Affanmir , are you referring to this :
Simply put if I want to send headers i.e let's say "device_location" as part of the headers I will get a CORS Access policy denied. cors plugin has a field "allow_headers" even after putting "*" and combining it with the grpc-web plugin I still get the error. The only way I've found to pass aditional headers is to modify the grpc-web plugin which is not the right solution.

@sheharyaar
Copy link
Contributor

@Affanmir , please checkout #10851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user responded wait for update wait for the author's response in this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants