Test for changing parentRef field in policies (AuthPolicy and RateLimitPolicy) #622
Conversation
emmaaroche
force-pushed
the
test-change-parentref
branch
2 times, most recently
from
db28248 to
4dbcc75
Compare
…itPolicy) Signed-off-by: emmaaroche <[email protected]>
emmaaroche
force-pushed
the
test-change-parentref
branch
from
4dbcc75 to
5edc14b
Compare
|
|
||
|
|
There was a problem hiding this comment.
Add pytestmark please so these tests are properly collected by pytest when run from the command line. See other tests in the testsuite for the reference. Here should be kuadrant_only and dnspolicy marks
P.S. custom mark definitions are stored in our pyproject.toml
| if kuadrant: | ||
| route = HTTPRoute.create_instance(gateway_b.cluster, blame("route-b"), gateway_b, {"app": module_label}) | ||
| else: | ||
| route = EnvoyVirtualRoute.create_instance(gateway_b.cluster, blame("route-b"), gateway_b) |
There was a problem hiding this comment.
If you add the pytest marks above, you will be dealing only with the kuadrant and httproute case here. So no if else will be needed
| route = HTTPRoute.create_instance(gateway_b.cluster, blame("route-b"), gateway_b, {"app": module_label}) | ||
| else: | ||
| route = EnvoyVirtualRoute.create_instance(gateway_b.cluster, blame("route-b"), gateway_b) | ||
| route.add_hostname(wildcard_domain2) |
There was a problem hiding this comment.
Wildcard hostname is defined inside the gateway and it is better to write the exact hostname in the according httproute, same as first gateway and httproute are done. You can find similar setup inside the singlecluster/authorino/operator/clusterwide/conftest.py. There is also a second client fixture that can help you in this test
| response = client.get("/get", auth=auth) | ||
| assert response.status_code == 200 | ||
|
|
||
| rate_limit_policy.wait_for_ready() |
There was a problem hiding this comment.
Mostly you should wait for the policy to become ready right after you commit it to the server. Or you did some changes in it and waiting for this policy reconciliation
| initial_target_ref = rate_limit_policy.model["spec"]["targetRef"]["name"] | ||
| assert ( | ||
| initial_target_ref == gateway.model.metadata.name | ||
| ), f"Initial targetRef mismatch: expected {gateway.model.metadata.name}, got {initial_target_ref}" |
There was a problem hiding this comment.
this check is a bit unnecessary. We expect that the server will have what we have commited there
| authorization.model["spec"]["targetRef"] = gateway.reference | ||
| authorization.apply() | ||
| authorization.wait_for_ready() | ||
| authorization.refresh() |
There was a problem hiding this comment.
Setup should be done inside the fixtures. If higher conftests set wrong targetRef then feel free to overwrite it here
| updated_target_ref = rate_limit_policy.model["spec"]["targetRef"]["name"] | ||
| assert ( | ||
| updated_target_ref == gateway_b.model.metadata.name | ||
| ), f"Updated targetRef mismatch: expected {gateway_b.model.metadata.name}, got {updated_target_ref}" |
There was a problem hiding this comment.
same as above, we expect it to be there
| rate_limit_policy.wait_for_ready() | ||
| rate_limit_policy.refresh() | ||
|
|
||
| rate_limit_policy.model["spec"]["targetRef"]["name"] = gateway_b.model.metadata.name |
There was a problem hiding this comment.
Openshift python library allows both dictionary and dot notations to access object values. Consider using only the dot notation for consistency. Here and everywhere else
| response = client.get("/get", auth=auth) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
This should work even without auth, because you're using the same client that points to the hostname from the first httproute. You will need second client fixture that will point on the new route
| response = client.get("/get", auth=auth) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
You need to check if ratelimit policy is correctly applied to the first gateway in the test beginning and is limiting the requests. get_many client method should help with it
Test for changing parentRef field in policies (AuthPolicy and RateLimitPolicy)