Conversation
80700f6 to
24acf6d
Compare
24acf6d to
ddc603e
Compare
c3df06a to
4f11377
Compare
9304fb3 to
2325864
Compare
|
/make smoke |
|
Test run completed ( Short Test SummaryFull Output |
|
/make smoke |
|
Test run completed ( Short Test SummaryFull Output |
azgabur
left a comment
There was a problem hiding this comment.
This PR has a lot of strange refactors I do not understand why are needed to achieve the PR's goal.
The suggested way to extend Policy class to add the wait for kuadrant_config update seems bit hack-y to me. The metrics are exposed in Gateway, so I recommend moving the code which handles the update check there. The Policies which are being updated/applied could have reference to their gateway stored as a field (similar how they store "cluster") and use it inside commit (to get current kuadrant_config generation number) and wait_for_* methods (to check the number was incremented).
I can see the problem in which we do commit fixture. In some situations (for example testsuite/tests/multicluster/coredns/two_clusters/conftest.py) there are multiple policies commited before their wait_for_ready methods are called. Meaning you cant expect that .commit() and .wait_for_ready() methods will be called right after each other so the generation field could be incremented by something else then the current policy waiting.
I do not know how the kuadrant_config structure looks like, but if it contains field about generation of specific policy being applied, you could use generation field of the policy similar how the current wait_for_ready works in Policy to wait until the gateway has the field in the metric.
In any case, this check seems like something that could be done on the Operator side and used to set the Ready/Enforced status more correctly, I would suggest that as a issue and have the implementation of such check in testsuite only if they reject implementing it.
There was a problem hiding this comment.
Why rename name -> _name ? And create duplicate name property?
testsuite/gateway/__init__.py
Outdated
| """Returns TLS cert bound to this Gateway, if the Gateway does not use TLS, returns None""" | ||
|
|
||
| @abstractmethod | ||
| def name(self) -> str: |
| class PlanPolicy(Policy): | ||
| """PlanPolicy object, used for applying plan-based policies to a Gateway/HTTPRoute""" | ||
|
|
||
| def __init__(self, *args, **kwargs): |
There was a problem hiding this comment.
Unrelated refactor as well as in testsuite/kuadrant/extensions/telemetry_policy.py
| def gateway(request, authorino, cluster, blame, module_label, testconfig, keycloak): | ||
| """Deploys Envoy with additional JWT plain identity test setup""" | ||
| envoy = JwtEnvoy( | ||
| envoy = JwtEnvoy( # pylint: disable=abstract-class-instantiated |
There was a problem hiding this comment.
JwtEnvoy is not abstract class, why is this disable here?
| self._hostname = hostname | ||
| self.verify = verify | ||
| self.ip_getter = ip_getter | ||
| self.verify = verify |
There was a problem hiding this comment.
it wasn't matching the parameters order.
|
|
||
| @pytest.fixture(scope="module", autouse=True) | ||
| def commit(request, commit, authorization2): # pylint: disable=unused-argument | ||
| def commit(request, authorization2): # pylint: disable=unused-argument |
There was a problem hiding this comment.
Why remove parent commit fixture here?
| def gateway(request, authorino, cluster, blame, label, testconfig) -> Envoy: | ||
| """Deploys Envoy that wires up the Backend behind the reverse-proxy and Authorino instance""" | ||
| gw = Envoy( | ||
| gw = Envoy( # pylint: disable=abstract-class-instantiated |
There was a problem hiding this comment.
Envoy is not abstract class, why is this disable here?
|
|
||
| @pytest.fixture(scope="module", autouse=True) | ||
| def commit(request, commit, wristband_authorization): # pylint: disable=unused-argument | ||
| def commit(request, wristband_authorization): # pylint: disable=unused-argument |
There was a problem hiding this comment.
Why remove parent commit fixture here?
| metrics_service.commit() | ||
| metrics_service.wait_for_ready() | ||
|
|
||
| return exposer.expose_hostname(blame("metrics"), metrics_service) |
There was a problem hiding this comment.
You dont need to create whole class MetricsServiceGateway just to run exposer on it. In the worst case this will create loadbalanced service targeting loadbalanced service doubling infrastructure strain which now mostly causes the sporadic failures.
I suggest using the Gateway's loadbalanced service which already exists to expose the port you need (15020) for the metrics. See https://istio.io/latest/docs/tasks/observability/metrics/secure-metrics/#secure-metrics-for-gateways
This will avoid whole exposing hassle.
| hostname, | ||
| backend, | ||
| module_label, | ||
| gateway_metrics_service, # pylint: disable=unused-argument |
bd66428 to
a82f6ad
Compare
d692c8d to
0b52dc5
Compare
0b52dc5 to
7125693
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
7125693 to
6b44322
Compare
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
Description
Sectionbase class to eliminate code duplication and prevent empty dict creationwait_for_ready()with Envoy-aware waiting mechanism using metrics validationChanges
New Infrastructure
testsuite/core/topology.py): 493-line system for tracking Gateway API resource relationships and dependenciesTopologyRegistrysingleton for global resource trackingTopologyNodefor representing resources and their relationships (targets, targeted_by, children, parent)@topologydecorator for automatic resource registrationtestsuite/core/metrics_factory.py): Factory pattern for creatingGatewayMetricsimplementations based on exposer typetestsuite/gateway/metrics.py): Abstract base class and implementations for LoadBalancer and OpenShift gateway metricstestsuite/kuadrant/policy/metric_validator.py): Base class for validating policy-related metricsRefactoring
testsuite/kuadrant/policy/__init__.py): GenericSectionbase class with:get_section()navigation for defaults/overrides and auth sectionsadd_to_spec()helper for dataclass conversioncommittedproperty delegation andmodify_and_apply()support@modifydecorator onstrategy()methodtestsuite/kuadrant/policy/rate_limit.py): UsesRateLimitSectionsubclass, eliminates code duplication inadd_limit()testsuite/kuadrant/policy/authorization/auth_policy.py): UsesAuthSectioncombiningSection+AuthConfigtestsuite/kuadrant/policy/authorization/sections.py): Updated to inherit from baseSectionclassEnhancements
testsuite/gateway/gateway_api/gateway.py): Addedwait_for_ready()with Envoy readiness validation via metricstestsuite/gateway/gateway_api/route.py): Addedwait_for_ready()delegating to gatewaytestsuite/kubernetes/service.py): Addedservice_url()methodget_metrics()method toExposerprotocoltestsuite/gateway/envoy/__init__.py): Implements metrics supportTests
wait_for_ready()calls in multiple test conftest files to ensure Envoy readiness before running testsVerification steps
All modified test files should be verified to ensure they work with the new infrastructure:
Closes: #846