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

[ISSUE #6968] fix grpc acl bug #6969

Merged
merged 5 commits into from
Jul 15, 2023
Merged

Conversation

lyx2000
Copy link
Contributor

@lyx2000 lyx2000 commented Jun 29, 2023

Which Issue(s) This PR Fixes

Fixes #6968

Brief Description

As issue mentioned.

How Did You Test This Change?

The failure examples mentioned in the issue have been successfully accessed, then I will add enough unit tests like remoting as soon as possible.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #6969 (89f13fc) into develop (4f840af) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             develop    #6969      +/-   ##
=============================================
- Coverage      42.65%   42.62%   -0.03%     
- Complexity      9234     9235       +1     
=============================================
  Files           1136     1136              
  Lines          80635    80644       +9     
  Branches       10538    10539       +1     
=============================================
- Hits           34393    34377      -16     
- Misses         41938    41976      +38     
+ Partials        4304     4291      -13     
Impacted Files Coverage Δ
...apache/rocketmq/acl/plain/PlainAccessResource.java 53.18% <0.00%> (+0.64%) ⬆️
...n/java/org/apache/rocketmq/proxy/ProxyStartup.java 42.62% <0.00%> (-2.21%) ⬇️
.../apache/rocketmq/proxy/grpc/GrpcServerBuilder.java 0.00% <ø> (ø)
...ocketmq/proxy/remoting/RemotingProtocolServer.java 0.00% <0.00%> (ø)

... and 39 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@drpmma drpmma left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe adding some unit tests would be better.

@lyx2000
Copy link
Contributor Author

lyx2000 commented Jun 30, 2023

LGTM, but maybe adding some unit tests would be better.

ok I will add some necessary to prove this

yuz10
yuz10 previously approved these changes Jul 1, 2023
@yuz10
Copy link
Member

yuz10 commented Jul 3, 2023

@lyx2000 Some check not pass, can you rerun the tests?

@lyx2000
Copy link
Contributor Author

lyx2000 commented Jul 3, 2023

@lyx2000 Some check not pass, can you rerun the tests?

Ok, please wait for me for a few days, I will add some unit tests that can reflect the problem. 💪

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@lyx2000
Copy link
Contributor Author

lyx2000 commented Jul 9, 2023

When writing the unit tests, I found that there wasn't a test complete with a joint test from parsing the request to checking the permissions, so I added tests related to both protocol clients. These additional tests would not have passed before my fix.

@lyx2000 lyx2000 requested review from yuz10, drpmma and mxsm July 9, 2023 15:11
Signed-off-by: lyx <[email protected]>

# Conflicts:
#	proxy/src/main/java/org/apache/rocketmq/proxy/grpc/GrpcServerBuilder.java
Signed-off-by: lyx <[email protected]>
@yuz10 yuz10 merged commit 5914ff8 into apache:develop Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Proxy ACL grpc parse PlainAccessResource wrong.
5 participants