-
Notifications
You must be signed in to change notification settings - Fork 26
test: Add backend not reached test #917
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||||
| """Test that checks if requests that are blocked by Authorino do not reach the backend""" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be helpful for traceability to link the bug issue Kuadrant/wasm-shim#321 directly in the test file. Somewhere inside a docstring or
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||||||
|
|
||||||
| import pytest | ||||||
|
|
||||||
| from testsuite.kuadrant.policy.authorization import DenyResponse, Value | ||||||
| from testsuite.utils import rego_allow_header | ||||||
|
|
||||||
|
|
||||||
| @pytest.fixture(scope="module") | ||||||
| def require_observability_disabled(kuadrant): | ||||||
| """ | ||||||
| Skip test if tracing is enabled in the Kuadrant CR. | ||||||
| This test does not verify properly with tracing enabled | ||||||
| """ | ||||||
| tracing_spec = kuadrant.model.spec.get("observability", {}).get("tracing") | ||||||
| if tracing_spec is not None and tracing_spec.get("defaultEndpoint") is not None: | ||||||
| pytest.skip("This test does not verify properly with tracing enabled") | ||||||
|
Comment on lines
+15
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I understand why tracing is an issue here. By default we enable tracing on all test clusters, if we keep it like that this test won't ever run, unless we disable/enable tracing for tests that require it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is an issue with a race condition additional processing from tracing was causing delays where the race condition window was already closed. To properly have the issue appear tracing must be turned off. Again I agree we enable tracing by default and this is the only test requiring tracing disabled so it is a problem. Could be solved by expanding testing matrix or having this test be a disruptive one which would disable/enable tracing. Im not much for the second option. But what do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not too sure either... But with tracing enabled is it making the issue harder to reproduce or not reproducible at all? Expanding the matrix would re-run the testsuite one more time, just with tracing enabled, I reckon? I am even considering that maybe tracing/observability should be disabled rather and test them separately. Bottom line is that this should never happen, with or without tracing. |
||||||
|
|
||||||
|
|
||||||
| @pytest.fixture(scope="module") | ||||||
| def authorization(authorization): | ||||||
| """ | ||||||
| Adds OPA policy that accepts all requests that contain `header` | ||||||
| Adds unauthorized message as it adds processing delay, increasing the chance for the bug to appear. | ||||||
| """ | ||||||
| authorization.identity.clear_all() | ||||||
| authorization.authorization.add_opa_policy("opa", rego_allow_header("key", "value")) | ||||||
| authorization.responses.set_unauthorized(DenyResponse(body=Value("Custom response"))) | ||||||
| return authorization | ||||||
|
Comment on lines
+21
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from what I understood, all types of failures could leak. I think we could also have a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed I agree to have test for RLP or TRLP as well, can be done in next PR, will create issue |
||||||
|
|
||||||
|
|
||||||
| @pytest.mark.issue("https://github.com/Kuadrant/wasm-shim/pull/321") | ||||||
| def test_backend_not_reached(client, require_observability_disabled): # pylint: disable=unused-argument | ||||||
| """ | ||||||
| Get current value of the counter. This counter increments by every request. | ||||||
| Do unauthorized requests which should not increment the counter. | ||||||
| Lastly check the counter if it contains expected value. | ||||||
| """ | ||||||
| before_counter = client.get("/counter", headers={"key": "value"}).json()["counter"] | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might not need to add the custom endpoint to httpbin, we have a MockserverBackend class with retrieve_requests method which should be doing the same thing. Also, from the quick search, there are specialized counting endpoints exist for this directly in the mockserver's api https://www.mock-server.com/mock_server/verification.html |
||||||
| client.get_many("/counter", 10) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
does it make sense to decrease number of api calls for this test?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With 10 requests I observed backend receiving about 7 so maybe 3 is enough, but this has minimal impact to keep it at 10 I think. |
||||||
| after_counter = client.get("/counter", headers={"key": "value"}).json()["counter"] | ||||||
| assert after_counter == before_counter + 1 | ||||||
|
Comment on lines
+39
to
+42
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thinking about the rate_limit variant, it might be better to get the counter on a different path, for example
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is another way do it yes, I went with simplicity over complexity when implementing the httpbin endpoint, but if you think it should be like that I can re-implement it. |
||||||
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.
Not sure why trying to run debugger on this file will fail for me. I think its location might be the issue.
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.
Hmm I didnt have problem with debugger, be sure to check where you have set "working directory", I remember that having it wrongly set the imports were not working correctly