-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat: As a user, I want to include the request body in the opa-input, so that I can reason about its contents #11629
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
base: master
Are you sure you want to change the base?
Conversation
|
Is there anything I can do to get this PR forward? |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions. |
|
@wistefan Please synchronize the master code to trigger all CI |
|
Hello! If I want this new implementation with the request body, what should I do? |
|
Hi @wistefan, please synchronize the latest master branch code to trigger the test. |
Hello! I tried to push a file to fix one of the issues, but I received a forbidden message. In any case, I solved it locally by running:
|
|
Regarding the error in t/discovery/consul_dump.t, I see that the test fails because the expected response from Consul isn’t being returned: It seems like either the service_a isn’t being registered properly in Consul during the test, or the endpoint isn’t responding as expected. As for the other warnings and errors like: These appear to be related to missing services (like Consul or Etcd) or SSL verification issues, but I’m not entirely sure how to fix it with certainty. |
|
Good! I could remove one issue, the other one remains! Any help?? I would need some guidance on how to fix it |
|
Maybe we can merge the master branch. |
|
| @@ -46,6 +46,7 @@ The `opa` Plugin can be used to integrate with [Open Policy Agent (OPA)](https:/ | |||
| | with_route | boolean | False | false | | When set to true, sends information about the current Route. | | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to modify Chinese documents synchronously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
docs/en/latest/plugins/opa.md
Outdated
| | with_route | boolean | False | false | | When set to true, sends information about the current Route. | | ||
| | with_service | boolean | False | false | | When set to true, sends information about the current Service. | | ||
| | with_consumer | boolean | False | false | | When set to true, sends information about the current Consumer. Note that this may send sensitive information like the API key. Make sure to turn it on only when you are sure it is safe. | | ||
| | with_body | boolean | False | false | | When set to true, sends the request body. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request body may contain sensitive information (passwords, API keys, etc.), so a security warning needs to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just added a security warning
t/plugin/opa3.t
Outdated
| POST /hello | ||
| hello world | ||
| --- response_body | ||
| hello world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to verify the body data received by OPA during the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add more tests to verify this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wistefan, could you please take a look? I’m running into some issues with Nginx and would appreciate your help. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @LuciaCabanillasRodriguez, do you need any help with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please submit additional fixes to ensure all CI tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wistefan, is there still time to deal with these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any update here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The author may be unable to continue processing this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logs for this run have expired and are no longer available.
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions. |
Baoyuantop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current integration test is not strict enough.
In ci/pod/opa/with_body.rego, the rule allow { request.method == "POST" } makes the policy allow any POST request, regardless of whether the body is successfully sent or matched. Since all your test cases in t/plugin/opa3.t use POST, they would pass even if the body was missing or corrupted.
To truly verify that the body is being transmitted and parsed correctly by OPA, please update the rego policy to check for a specific value in the body (e.g., input.request.body.hello == "world"), and add a negative test case (e.g., sending `{"hello": "wrong"}) to ensure it gets blocked.
|
Could you please check the 2 failing checks? I think they are not related with opa |
|
Could you check it? I think the failing checks are not related with opa |
|
I reran the failed test. |
|
Can three people with write access review this, please? |
Description
In order to make decisions based on the request body, the OPA-Plugin will also forward the body when configured to do so.
Fixes #11387
Checklist