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.

Sign up for GitHub

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

Jump to bottom

fix: grpc-web trailers #10851

Merged
merged 8 commits into from
Jan 26, 2024
Merged

fix: grpc-web trailers #10851

merged 8 commits into from
Jan 26, 2024

Conversation

sheharyaar
Copy link
Contributor

@sheharyaar sheharyaar commented Jan 18, 2024 •
edited
Loading

Description

Fixes #10573 and closes #10613

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)

@sheharyaar
Copy link
Contributor Author

This PR is not yet complete, need to add some features.
@Affanmir, please checkout this PR and confirm if this fixes your issue ?

@sheharyaar sheharyaar changed the title fix grpc-web trailers fix: grpc-web trailers Jan 18, 2024
This was referenced Jan 18, 2024
Closed
Closed
@Vacant2333
Copy link
Contributor

@sheharyaar hello~ have u test on your enviorment? u can add some testcase that can help us to review~~

@sheharyaar
Copy link
Contributor Author

@Vacant2333 , I have added test-cases, please approve workflow

@sheharyaar sheharyaar mentioned this pull request Jan 19, 2024
Closed
@monkeyDluffy6017
Copy link
Contributor

@sheharyaar Is this pr ready?

@sheharyaar
Copy link
Contributor Author

Yes, the PR is ready @monkeyDluffy6017

@Affanmir
Copy link

This PR is not yet complete, need to add some features. @Affanmir, please checkout this PR and confirm if this fixes your issue ?

Missing trailers issue fixed but path based testing cannot be done until the cors issue is resolved.

Access to XMLHttpRequest at 'https://XXXXXXXX/RANDOM.RANDOM.SERVICENAME.service.ServiceExternal/SERVICEMETHOF' from origin 'http://localhost:8080/' has been blocked by CORS policy: Request header field REDACTED_FIELD is not allowed by Access-Control-Allow-Headers in preflight response.
SUBDOMAON.DOMAIN/RANDOM.RANDOM.SERVICENAME.service.ServiceExternal/SERVICEMETHOD:1

The issue persistents even when using cors plugin alongside this plugin. Please replicate on your end

shreemaan-abhishek
shreemaan-abhishek reviewed Jan 22, 2024
View reviewed changes
apisix/plugins/grpc-web.lua Outdated Show resolved Hide resolved
apisix/plugins/grpc-web.lua Show resolved Hide resolved
apisix/plugins/grpc-web.lua Show resolved Hide resolved
Vacant2333
Vacant2333 suggested changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@Vacant2333 Vacant2333 left a comment

Choose a reason for hiding this comment

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

hi, the ci was failed mostly, u can merge the master after the pr get merged~ it will fix it

@Vacant2333
Copy link
Contributor

Vacant2333 commented Jan 23, 2024 •
edited
Loading

r u ready to review and merge now? @shreemaan-abhishek can u help take a look~

@sheharyaar
Copy link
Contributor Author

Yes, I am ready

shreemaan-abhishek
shreemaan-abhishek requested changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

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

https://github.com/apache/apisix/pull/10851/files#r1463630260

shreemaan-abhishek
shreemaan-abhishek requested changes Jan 24, 2024
View reviewed changes
t/plugin/grpc-web.t Outdated Show resolved Hide resolved
t/plugin/grpc-web.t Outdated Show resolved Hide resolved
@Revolyssup
Copy link
Contributor

please fix ci

@sheharyaar
Copy link
Contributor Author

I am having issues with this. Locally it works fine for me. I need help with this.

@sheharyaar
fix grpc-web trailers
ff775cb 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: grpc-web trailer logic and add test case
080839d 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: workflow
66977ab 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: remove tonumber
7a4ebed 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: lint
07812e4 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: add test case
38a722c 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: ci and lint
77cc6a8 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@sheharyaar
fix: minor change
1e17947 
Signed-off-by: Mohammad Shehar Yaar Tausif <[email protected]>
@monkeyDluffy6017 monkeyDluffy6017 merged commit 0bea3db into apache:master Jan 26, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vacant2333 Vacant2333 Vacant2333 requested changes

@shreemaan-abhishek shreemaan-abhishek shreemaan-abhishek approved these changes

@Affanmir Affanmir Affanmir approved these changes

@monkeyDluffy6017 monkeyDluffy6017 monkeyDluffy6017 approved these changes

@Revolyssup Revolyssup Awaiting requested review from Revolyssup

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: grpc-web plugin causing missing trailers issue
6 participants
@sheharyaar @Vacant2333 @monkeyDluffy6017 @Affanmir @Revolyssup @shreemaan-abhishek