refactor: remove xfails and incorporate review comments

Signed-off-by: Silvia Tarabova <starabov@redhat.com>

Author: silvi-t

testsuite/tests/singlecluster/extensions/pipeline_policy/conftest.py

@@ -2,6 +2,8 @@
 
 import pytest
 
+from openshift_client import OpenShiftPythonException
+
 from testsuite.kubernetes import Selector
 from testsuite.kubernetes.deployment import Deployment
 from testsuite.kubernetes.service import Service, ServicePort
@@ -13,7 +15,7 @@ def check_pipeline_policy_crd(cluster, skip_or_fail):
     """Skip all PipelinePolicy tests if the CRD is not installed on the cluster."""
     try:
         cluster.do_action("get", "crd/pipelinepolicies.extensions.kuadrant.io")
-    except Exception:  # pylint: disable=broad-except
+    except OpenShiftPythonException:
         skip_or_fail("PipelinePolicy CRD is not installed on the cluster")
 
 
@@ -57,7 +59,6 @@ def threat_assessment_service(request, cluster, blame, module_label, testconfig)
 @pytest.fixture(scope="module", autouse=True)
 def commit(request, pipeline_policy):
     """Commit and wait for PipelinePolicy to be ready."""
-    for component in [pipeline_policy]:
-        request.addfinalizer(component.delete)
-        component.commit()
-        component.wait_for_ready()
+    request.addfinalizer(pipeline_policy.delete)
+    pipeline_policy.commit()
+    pipeline_policy.wait_for_ready()

testsuite/tests/singlecluster/extensions/pipeline_policy/interactions/test_pipeline_policy_auth.py

@@ -36,8 +36,6 @@ def test_auth_and_pipeline_unauthorized(client):
     assert response.headers.get("x-pipeline-policy") is None
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/wasm-shim/issues/371")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/wasm-shim/issues/371")
 def test_auth_and_pipeline_blocked_path(client, auth):
     """Authenticated request to blocked path is denied by PipelinePolicy deny action."""
     response = client.get("/blocked", auth=auth)

testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_composition.py

@@ -49,8 +49,6 @@ def test_fail_before_deny(request, cluster, blame, route, client, threat_assessm
     assert response.status_code == 500
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/wasm-shim/issues/371")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/wasm-shim/issues/371")
 def test_deny_after_grpc_call(request, cluster, blame, route, client, threat_assessment_service):
     """Deny action after gRPC call works when the deny predicate matches."""
     svc_url = (
@@ -100,8 +98,6 @@ def test_deny_based_on_grpc_var(request, cluster, blame, route, client, threat_a
     assert response.status_code == 200
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018")
 def test_deny_with_dynamic_body(request, cluster, blame, route, client, threat_assessment_service):
     """Deny action with CEL expression in withBody interpolates gRPC response variable."""
     svc_url = (

testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_deny.py

@@ -92,17 +92,13 @@ def test_deny_custom_headers(client):
     assert response.headers.get("x-deny-reason") == "blocked"
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018")
 def test_deny_custom_body(client):
     """Deny with withBody as CEL expression returns custom body text."""
     response = client.get("/custom-body")
     assert response.status_code == 403
     assert response.text == "Access denied"
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018")
 def test_deny_all_response_fields(client):
     """Deny with withStatus, withHeaders, and withBody as CEL expression returns all fields."""
     response = client.get("/custom-all")
@@ -123,8 +119,6 @@ def test_response_deny_no_override(client):
     assert response.status_code == 200
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2018")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2018")
 def test_response_deny_with_headers_and_body(client):
     """Response deny with all fields as CEL expressions replaces the backend response."""
     response = client.get("/get", headers={"x-resp-deny": "true"})

testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_isolation.py

@@ -80,28 +80,27 @@ def pipeline_policy(pipeline_policy):
     return pipeline_policy
 
 
-def test_policy_affects_targeted_route(client):
-    """Route with PipelinePolicy gets the response header."""
+def test_policy_does_not_affect_other_route(client, client2):
+    """Route without PipelinePolicy on the same gateway does not get the response header."""
+    time.sleep(EXTENSION_POLICY_PROPAGATION_WAIT)
+
     response = client.get("/get")
     assert response.status_code == 200
     assert response.headers.get("x-pipeline-policy") == "active"
 
-
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2023")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2023")
-def test_policy_does_not_affect_other_route(client2):
-    """Route without PipelinePolicy on the same gateway does not get the response header."""
-    time.sleep(EXTENSION_POLICY_PROPAGATION_WAIT)
     response = client2.get("/get")
     assert response.status_code == 200
     assert response.headers.get("x-pipeline-policy") is None
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2023")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2023")
-def test_policy_does_not_affect_other_gateway(client3):
+def test_policy_does_not_affect_other_gateway(client, client3):
     """Route on a different gateway does not get the response header."""
     time.sleep(EXTENSION_POLICY_PROPAGATION_WAIT)
+
+    response = client.get("/get")
+    assert response.status_code == 200
+    assert response.headers.get("x-pipeline-policy") == "active"
+
     response = client3.get("/get")
     assert response.status_code == 200
     assert response.headers.get("x-pipeline-policy") is None

testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_lifecycle.py

@@ -1,8 +1,11 @@
 """Tests for PipelinePolicy lifecycle: update and delete."""
 
+import time
+
 import pytest
 
 from testsuite.kuadrant.extensions.pipeline_policy import PipelinePolicy
+from testsuite.utils.constants import EXTENSION_POLICY_PROPAGATION_WAIT
 
 pytestmark = [pytest.mark.kuadrant_only, pytest.mark.extensions]
 
@@ -35,8 +38,6 @@ def test_update_policy(request, cluster, blame, route, client):
     assert response.headers.get("x-update-new") == "true"
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2009")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2009")
 @pytest.mark.flaky(reruns=0)
 def test_delete_policy(request, cluster, blame, route, client):
     """After deleting the PipelinePolicy, the CR is removed and the actions stop being enforced."""
@@ -51,7 +52,8 @@ def test_delete_policy(request, cluster, blame, route, client):
     assert response.headers.get("x-delete-test") == "active"
 
     policy.delete()
-    assert policy.wait_until(lambda obj: not obj.exists(), timelimit=30), "PipelinePolicy was not deleted"
+    assert not policy.committed, "PipelinePolicy was not deleted"
+    time.sleep(EXTENSION_POLICY_PROPAGATION_WAIT)
 
     response = client.get("/get")
     assert response.status_code == 200

testsuite/tests/singlecluster/extensions/pipeline_policy/test_pipeline_policy_validation.py

@@ -14,14 +14,14 @@ def commit():
     """No module-level policy; each test creates its own with bad configuration."""
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2022")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2022")
-def test_invalid_target_ref(request, cluster, blame):
-    """PipelinePolicy targeting a non-existent HTTPRoute does not reach Enforced state."""
+@pytest.mark.parametrize("kind", ["HTTPRoute", "Gateway"])
+def test_invalid_target_ref(request, cluster, blame, kind):
+    """PipelinePolicy targeting a non-existent resource does not reach Accepted state."""
+    target_name = "does-not-exist"
     target = CustomReference(
         group="gateway.networking.k8s.io",
-        kind="HTTPRoute",
-        name="does-not-exist",
+        kind=kind,
+        name=target_name,
     )
     policy = PipelinePolicy.create_instance(cluster, blame("bad-target"), target)
     policy.on_http_request.add_deny(predicate='request.url_path == "/blocked"', with_status=403)
@@ -30,34 +30,11 @@ def test_invalid_target_ref(request, cluster, blame):
     policy.commit()
 
     assert policy.wait_until(
-        has_condition("Accepted", "False", "TargetNotFound"),
+        has_condition("Accepted", "False", message=f"targetRef {kind} {cluster.project}/{target_name} not found"),
         timelimit=30,
-    ), f"Policy did not report TargetNotFound, status: {policy.refresh().model.status.conditions}"
+    ), f"Policy did not report target not found, status: {policy.refresh().model.status.conditions}"
 
 
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2022")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2022")
-def test_invalid_gateway_target_ref(request, cluster, blame):
-    """PipelinePolicy targeting a non-existent Gateway does not reach Enforced state."""
-    target = CustomReference(
-        group="gateway.networking.k8s.io",
-        kind="Gateway",
-        name="does-not-exist",
-    )
-    policy = PipelinePolicy.create_instance(cluster, blame("bad-gw"), target)
-    policy.on_http_request.add_deny(predicate='request.url_path == "/blocked"', with_status=403)
-
-    request.addfinalizer(policy.delete)
-    policy.commit()
-
-    assert policy.wait_until(
-        has_condition("Accepted", "False", "TargetNotFound"),
-        timelimit=30,
-    ), f"Policy did not report TargetNotFound, status: {policy.refresh().model.status.conditions}"
-
-
-@pytest.mark.issue("https://github.com/Kuadrant/kuadrant-operator/issues/2015")
-@pytest.mark.xfail(reason="https://github.com/Kuadrant/kuadrant-operator/issues/2015")
 def test_top_level_fail_action(request, cluster, blame, route):
     """PipelinePolicy with a top-level fail action (not inside gRPC onReply) should not be accepted."""
     policy = PipelinePolicy.create_instance(cluster, blame("top-fail"), route)
@@ -66,9 +43,8 @@ def test_top_level_fail_action(request, cluster, blame, route):
     request.addfinalizer(policy.delete)
     policy.commit()
 
-    # TODO: add expected message assertion once the validation is implemented
     assert policy.wait_until(
-        has_condition("Accepted", "False"),
+        has_condition("Accepted", "False", message="fail action must reference a gRPC response variable"),
         timelimit=30,
     ), f"Policy with top-level fail was accepted, status: {policy.refresh().model.status.conditions}"