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

Add tests for D&O merge strategy #638

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions testsuite/kuadrant/policy/__init__.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
"""Contains Base class for policies"""

from dataclasses import dataclass
from enum import Enum

from testsuite.kubernetes import KubernetesObject
from testsuite.utils import check_condition


class Strategy(Enum):
"""Class for merge strategies of defaults and overrides."""

ATOMIC = "atomic"
MERGE = "merge"


@dataclass
class CelPredicate:
"""Dataclass that references CEL predicate e.g. auth.identity.anonymous == 'true'"""
Expand Down
14 changes: 13 additions & 1 deletion testsuite/kuadrant/policy/authorization/auth_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from testsuite.utils import asdict
from .auth_config import AuthConfig
from .sections import ResponseSection
from .. import Policy, CelPredicate
from .. import Policy, CelPredicate, Strategy
from . import Pattern


Expand All @@ -27,6 +27,7 @@ def create_instance(
name,
target: Referencable,
labels: Dict[str, str] = None,
section_name: str = None,
):
"""Creates base instance"""
model: Dict = {
Expand All @@ -37,6 +38,8 @@ def create_instance(
"targetRef": target.reference,
},
}
if section_name:
model["spec"]["targetRef"]["sectionName"] = section_name

return cls(model, context=cluster.context)

Expand All @@ -46,6 +49,15 @@ def add_rule(self, when: list[CelPredicate]):
self.model.spec.setdefault("when", [])
self.model.spec["when"].extend([asdict(x) for x in when])

@modify
def strategy(self, strategy: Strategy) -> None:
"""Add strategy type to default or overrides spec"""
if self.spec_section is None:
raise TypeError("Strategy can only be set on defaults or overrides")

self.spec_section["strategy"] = strategy.value
self.spec_section = None
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of setting it to None here?


@property
def auth_section(self):
if self.spec_section is None:
Expand Down
24 changes: 21 additions & 3 deletions testsuite/kuadrant/policy/rate_limit.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from testsuite.gateway import Referencable
from testsuite.kubernetes import modify
from testsuite.kubernetes.client import KubernetesClient
from testsuite.kuadrant.policy import Policy, CelPredicate, CelExpression
from testsuite.kuadrant.policy import Policy, CelPredicate, CelExpression, Strategy
from testsuite.utils import asdict


Expand All @@ -27,16 +27,25 @@ def __init__(self, *args, **kwargs):
self.spec_section = None

@classmethod
def create_instance(cls, cluster: KubernetesClient, name, target: Referencable, labels: dict[str, str] = None):
def create_instance(
cls,
cluster: KubernetesClient,
name,
target: Referencable,
section_name: str = None,
labels: dict[str, str] = None,
):
"""Creates new instance of RateLimitPolicy"""
model = {
model: dict = {
"apiVersion": "kuadrant.io/v1",
"kind": "RateLimitPolicy",
"metadata": {"name": name, "labels": labels},
"spec": {
"targetRef": target.reference,
},
}
if section_name:
model["spec"]["targetRef"]["sectionName"] = section_name

return cls(model, context=cluster.context)

Expand All @@ -63,6 +72,15 @@ def add_limit(
self.spec_section.setdefault("limits", {})[name] = limit
self.spec_section = None

@modify
def strategy(self, strategy: Strategy) -> None:
"""Add strategy type to default or overrides spec"""
if self.spec_section is None:
raise TypeError("Strategy can only be set on defaults or overrides")

self.spec_section["strategy"] = strategy.value
self.spec_section = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Same again, unclear to me why there is this "None" assignment


@property
def defaults(self):
"""Add new rule into the `defaults` RateLimitPolicy section"""
Expand Down
Empty file.
12 changes: 12 additions & 0 deletions testsuite/tests/singlecluster/defaults/merge/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""Conftest for defaults merge strategy tests"""

import pytest


@pytest.fixture(scope="module")
def route(route, backend):
"""Add 2 backend rules for specific backend paths"""
route.remove_all_rules()
route.add_backend(backend, "/get")
route.add_backend(backend, "/anything")
return route
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Setup conftest for policy merge on the same targets"""

import pytest

from testsuite.kuadrant.policy import CelPredicate, Strategy
from testsuite.kuadrant.policy.rate_limit import Limit, RateLimitPolicy

LIMIT = Limit(4, "10s")
MERGE_LIMIT = Limit(2, "10s")
MERGE_LIMIT2 = Limit(6, "10s")


@pytest.fixture(scope="module")
def rate_limit(cluster, blame, module_label, route):
"""Add a RateLimitPolicy targeting the first HTTPRouteRule."""
rate_limit = RateLimitPolicy.create_instance(cluster, blame("sp"), route, labels={"testRun": module_label})
rate_limit.add_limit("basic", [LIMIT], when=[CelPredicate("request.path == '/get'")])
return rate_limit


@pytest.fixture(scope="module")
def default_merge_rate_limit(cluster, blame, module_label, route):
"""Add a RateLimitPolicy targeting the first HTTPRouteRule."""
policy = RateLimitPolicy.create_instance(cluster, blame("dmp"), route, labels={"testRun": module_label})
policy.defaults.add_limit("basic", [MERGE_LIMIT], when=[CelPredicate("request.path == '/get'")])
policy.defaults.add_limit("merge", [MERGE_LIMIT2], when=[CelPredicate("request.path == '/anything'")])
policy.defaults.strategy(Strategy.MERGE)
return policy
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Test defaults policy aimed at the same resoure uses oldested policy."""
Copy link
Contributor

Choose a reason for hiding this comment

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

resoure / oldested - typos?


import pytest

from .conftest import MERGE_LIMIT, MERGE_LIMIT2

pytestmark = [pytest.mark.kuadrant_only, pytest.mark.limitador]


@pytest.fixture(scope="module", autouse=True)
def commit(request, route, rate_limit, default_merge_rate_limit): # pylint: disable=unused-argument
"""Commits RateLimitPolicy after the HTTPRoute is created"""
for policy in [rate_limit, default_merge_rate_limit]: # Forcing order of creation.
request.addfinalizer(policy.delete)
policy.commit()
policy.wait_for_accepted()


def test_multiple_policies_merge_default_ab(client):
"""Test RateLimitPolicy with merge defaults being ignored due to age"""
responses = client.get_many("/get", MERGE_LIMIT.limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I think that the comments need to be swapped between test_ab_strategy and test_ba_strategy. If I got it right "a" means standard policy and "b" means policy with merge defaults.

So in "ab" the "a" policy is created first, and once "b" is created the "a" gets overridden - you can see it in test that "MERGE_LIMIT" is used for both get_many calls. However, the comment in "ab" suggests that "b" is ignored which is not true - it is true in the "ba" test where the policies are created in different order.

Copy link
Contributor

Choose a reason for hiding this comment

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

I decrypted the policy names.
dmp means policy with defauls section and merge strategy (defaults merge policy)
sp means standard policy === policy without both defaults and overrides sections
omp means policy with overrides section and merge strategy (overrides merge policy)

responses.assert_all(status_code=200)
assert client.get("/get").status_code == 429

responses = client.get_many("/anything", MERGE_LIMIT2.limit)
responses.assert_all(status_code=200)
assert client.get("/anything").status_code == 429
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Test defaults policy aimed at the same resoure uses oldested policy."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Typos here too


import pytest

from .conftest import LIMIT, MERGE_LIMIT2

pytestmark = [pytest.mark.kuadrant_only, pytest.mark.limitador]


@pytest.fixture(scope="module", autouse=True)
def commit(request, route, rate_limit, default_merge_rate_limit): # pylint: disable=unused-argument
"""Commits RateLimitPolicy after the HTTPRoute is created"""
for policy in [default_merge_rate_limit, rate_limit]: # Forcing order of creation.
request.addfinalizer(policy.delete)
policy.commit()
policy.wait_for_accepted()


def test_multiple_policies_merge_default_ba(client):
"""Test RateLimitPolicy with merge defaults being enforced due to age"""
responses = client.get_many("/get", LIMIT.limit)
responses.assert_all(status_code=200)
assert client.get("/get").status_code == 429

responses = client.get_many("/anything", MERGE_LIMIT2.limit)
responses.assert_all(status_code=200)
assert client.get("/anything").status_code == 429
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
"""Test gateway level default merging with and being patrially overriden by another policy."""
Copy link
Contributor

Choose a reason for hiding this comment

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

*partially overridden


import pytest

from testsuite.kuadrant.policy import CelPredicate
from testsuite.kuadrant.policy.rate_limit import RateLimitPolicy, Limit, Strategy

pytestmark = [pytest.mark.kuadrant_only, pytest.mark.limitador]


@pytest.fixture(scope="module")
def rate_limit(rate_limit):
"""Create a RateLimitPolicy with a basic limit with same target as one default."""
when = CelPredicate("request.path == '/get'")
rate_limit.add_limit("route_limit", [Limit(5, "5s")], when=[when])
return rate_limit


@pytest.fixture(scope="module")
def global_rate_limit(cluster, blame, module_label, gateway):
"""Create a RateLimitPolicy with default policies and a merge strategy."""
global_rate_limit = RateLimitPolicy.create_instance(
cluster, blame("limit"), gateway, labels={"testRun": module_label}
)
gateway_when = CelPredicate("request.path == '/anything'")
global_rate_limit.defaults.add_limit("gateway_limit", [Limit(3, "5s")], when=[gateway_when])
route_when = CelPredicate("request.path == '/get'")
global_rate_limit.defaults.add_limit("route_limit", [Limit(10, "5s")], when=[route_when])
global_rate_limit.defaults.strategy(Strategy.MERGE)
return global_rate_limit


@pytest.fixture(scope="module", autouse=True)
def commit(request, route, rate_limit, global_rate_limit): # pylint: disable=unused-argument
"""Commits RateLimitPolicy after the HTTPRoute is created"""
for policy in [global_rate_limit, rate_limit]: # Forcing order of creation.
request.addfinalizer(policy.delete)
policy.commit()
policy.wait_for_ready()


@pytest.mark.parametrize("rate_limit", ["gateway", "route"], indirect=True)
def test_gateway_default_merge(client):
"""Test Gateway default policy being partially overriden when another policy with the same target is created."""
get = client.get_many("/get", 5)
get.assert_all(status_code=200)
assert client.get("/get").status_code == 429

anything = client.get_many("/anything", 3)
anything.assert_all(status_code=200)
assert client.get("/anything").status_code == 429
Empty file.
12 changes: 12 additions & 0 deletions testsuite/tests/singlecluster/overrides/merge/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""Conftest for overrides merge strategy tests"""

import pytest


@pytest.fixture(scope="module")
def route(route, backend):
"""Add 2 backend rules for specific backend paths"""
route.remove_all_rules()
route.add_backend(backend, "/get")
route.add_backend(backend, "/anything")
return route
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Setup conftest for policy override on the same targets"""

import pytest

from testsuite.kuadrant.policy import CelPredicate, Strategy
from testsuite.kuadrant.policy.rate_limit import Limit, RateLimitPolicy

LIMIT = Limit(4, "5s")
OVERRIDE_LIMIT = Limit(6, "5s")
OVERRIDE_LIMIT2 = Limit(2, "5s")


@pytest.fixture(scope="module")
def rate_limit(cluster, blame, module_label, route):
"""Add a RateLimitPolicy targeting the first HTTPRouteRule."""
rate_limit = RateLimitPolicy.create_instance(cluster, blame("sp"), route, labels={"testRun": module_label})
rate_limit.add_limit("basic", [LIMIT], when=[CelPredicate("request.path == '/get'")])
return rate_limit


@pytest.fixture(scope="module")
def override_merge_rate_limit(cluster, blame, module_label, route):
"""Add a RateLimitPolicy targeting the first HTTPRouteRule."""
policy = RateLimitPolicy.create_instance(cluster, blame("omp"), route, labels={"testRun": module_label})
policy.overrides.add_limit("basic", [OVERRIDE_LIMIT], when=[CelPredicate("request.path == '/get'")])
policy.overrides.add_limit("override", [OVERRIDE_LIMIT2], when=[CelPredicate("request.path == '/anything'")])
policy.overrides.strategy(Strategy.MERGE)
return policy
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Test override policies aimed at the same resoure uses oldested policy."""

import pytest

from .conftest import OVERRIDE_LIMIT, OVERRIDE_LIMIT2

pytestmark = [pytest.mark.kuadrant_only, pytest.mark.limitador]


@pytest.fixture(scope="module", autouse=True)
def commit(request, route, rate_limit, override_merge_rate_limit): # pylint: disable=unused-argument
"""Commits RateLimitPolicy after the HTTPRoute is created"""
for policy in [rate_limit, override_merge_rate_limit]: # Forcing order of creation.
request.addfinalizer(policy.delete)
policy.commit()
policy.wait_for_accepted()


def test_multiple_policies_merge_default_ab(client):
"""Test RateLimitPolicy with merge overrides being ignored due to age"""
Copy link
Contributor

Choose a reason for hiding this comment

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

The RateLimitPolicy with merge overrides is not ignored. On the contrary it is fully enforced

Copy link
Contributor

Choose a reason for hiding this comment

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

What is also weird is the fact that "ab" and "ba" strategy are the same, ie the order of creation of the polices does not matter, in both cases the "b" (one with overrides section and merge strategy) is fully enforced and "a" is overridden.
Not sure if this is expected behavior or not.

responses = client.get_many("/get", OVERRIDE_LIMIT.limit)
responses.assert_all(status_code=200)
assert client.get("/get").status_code == 429

responses = client.get_many("/anything", OVERRIDE_LIMIT2.limit)
responses.assert_all(status_code=200)
assert client.get("/anything").status_code == 429
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
"""Test override policy aimed at the same resoure uses oldested policy."""

import pytest

from .conftest import OVERRIDE_LIMIT, OVERRIDE_LIMIT2

pytestmark = [pytest.mark.kuadrant_only, pytest.mark.limitador]


@pytest.fixture(scope="module", autouse=True)
def commit(request, route, rate_limit, override_merge_rate_limit): # pylint: disable=unused-argument
"""Commits RateLimitPolicy after the HTTPRoute is created"""
for policy in [override_merge_rate_limit, rate_limit]: # Forcing order of creation.
request.addfinalizer(policy.delete)
policy.commit()
policy.wait_for_accepted()


def test_multiple_policies_merge_default_ba(client):
"""Test RateLimitPolicy with merge overrides being enforced due to age"""
responses = client.get_many("/get", OVERRIDE_LIMIT.limit)
responses.assert_all(status_code=200)
assert client.get("/get").status_code == 429

responses = client.get_many("/anything", OVERRIDE_LIMIT2.limit)
responses.assert_all(status_code=200)
assert client.get("/anything").status_code == 429
Loading