-
Notifications
You must be signed in to change notification settings - Fork 23
refactor: SpiceDB test #808
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?
Conversation
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Outdated
Show resolved
Hide resolved
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Outdated
Show resolved
Hide resolved
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Outdated
Show resolved
Hide resolved
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Outdated
Show resolved
Hide resolved
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Outdated
Show resolved
Hide resolved
azgabur
left a comment
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.
If the purpose of this PR is just to allow parallel runs of the same test I think only modification of fixtures schema_config and relationship_config was needed or maybe some other places. Currently (on main) the configs uses static names for all fields which I presume would override each other. Not sure how the spiceDB handles writes of the same schemas. Did you try making the names of the schemas unique between runs?
testsuite/backend/spicedb.py
Outdated
| ServicePort(name="grpc", port=50051, targetPort="grpc"), | ||
| ], | ||
| labels={"app": self.label}, | ||
| service_type="LoadBalancer", |
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.
Is the service used outside the cluster like multicluster test? If not, we maybe don't need to use LoadBalancer type here. This could also apply to some services in tools. Needs investigation.
Especially if you want to choose hard-coded secret.
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.
As far as I understand, we used loadbalancer services for tools to replicate customers' behavior. So it makes sense for me to have spicedb loadbalanced as it is an authorization service supposed to be used together with kuadrant, but it doesn't make sense for me to have mockserver loadbalanced for example.
The problem is that the schema in SpiceDB only overwrites itself, and using unique names does not solve this problem. Therefore, you cannot have multiple schemas in a single SpiceDB instance. That is why we want to create a single instance for each test. |
5964f57 to
0c10f12
Compare
0c10f12 to
7b6a524
Compare
|
I've added a commit to handle SpiceDB connection issues related to LoadBalancer provisioning and I would like to hear your thoughts on the solution @azgabur, @averevki:
The retry logic gives the LoadBalancer time to become accessible on rhos-01 clusters, and if it remains unreachable after retries, the test is skipped with a clear explanation rather than producing a false test failure. |
|
Loadbalancer problems on Openstack are indeed infrastructure problem. If SpiceDB is not used in multicluster testing maybe using local service url would suffice? |
7b6a524 to
a4d1447
Compare
@azgabur I've taken your suggestion to use local service URLs instead of relying on LoadBalancer IPs. I've changed the approach to route to SpiceDB through an HTTPRoute via the Gateway. This eliminates the LoadBalancer reliability issues on OpenStack entirely. The reason I had to switch to an HTTP client is that the official Regarding deploying instances at runtime versus using SpiceDB in tools - I actually think having one instance per test is better in this case. SpiceDB doesn't support running multiple schemas in a single instance, which |
testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
Show resolved
Hide resolved
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
a4d1447 to
1a55de4
Compare
averevki
left a comment
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.
LGTM
azgabur
left a comment
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 the library was removed in favor of direct http client but it works and I dont mind.
| max_tries=3, | ||
| interval=5, | ||
| jitter=None, | ||
| on_giveup=lambda details: (_ for _ in ()).throw( |
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.
what is the (_ for _ in ()) for? Seems like a no-op
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.
The (_ for _ in ()).throw(...) pattern is not a no-op - it's a working technique to raise exceptions from lambda expressions. It creates a generator object and calls its .throw() method to raise an exception, which is a workaround for the fact that lambda functions can only contain expressions, not raise statements.
| """ | ||
| Create an OpenShift Route to SpiceDB for external access from test suite. | ||
| """ | ||
| route = OpenshiftRoute.create_instance(cluster, blame("spicedb"), service_name=spicedb.name, target_port="http") |
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.
Using OpenshiftRoute is not encouraged in testsuite because it only works on Openshift. Not kubernetes or kind. I now see why you need to access the spicedb externally, because you are configuring it via http requests from testsuite. Its similar to the MockserverBackend. I think loadbalancer service should be used here instead. I tried it like this and it worked:
From 9c25dfe35bb25bab990c312073d9198db8a97361 Mon Sep 17 00:00:00 2001
From: Alex Zgabur <azgabur@redhat.com>
Date: Tue, 6 Jan 2026 14:44:00 +0100
Subject: [PATCH] TEST
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
---
testsuite/spicedb/spicedb.py | 5 ++++-
.../authorino/authorization/spicedb/test_spicedb.py | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/testsuite/spicedb/spicedb.py b/testsuite/spicedb/spicedb.py
index f5aa21e..2a30010 100644
--- a/testsuite/spicedb/spicedb.py
+++ b/testsuite/spicedb/spicedb.py
@@ -5,6 +5,7 @@ from typing import List
import backoff
import httpx
+from testsuite.config import settings
from testsuite.backend import Backend
from testsuite.kubernetes import Selector
from testsuite.kubernetes.client import KubernetesClient
@@ -256,12 +257,14 @@ class SpiceDB(Backend):
selector=match_labels,
ports=[
ServicePort(name="grpc", port=50051, targetPort="grpc"),
- ServicePort(name="http", port=8443, targetPort="http"),
+ ServicePort(name="http", port=80, targetPort="http"),
],
labels={"app": self.label},
+ service_type="LoadBalancer",
)
self.service.commit()
def wait_for_ready(self, timeout=60 * 5):
"""Waits until Deployment is marked as ready"""
self.deployment.wait_for_ready(timeout)
+ self.service.wait_for_ready(timeout, settings["control_plane"]["slow_loadbalancers"])
diff --git a/testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py b/testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
index ca08030..c68e78a 100644
--- a/testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
+++ b/testsuite/tests/singlecluster/authorino/authorization/spicedb/test_spicedb.py
@@ -68,7 +68,7 @@ def spicedb_route(request, spicedb, blame, cluster):
request.addfinalizer(route.delete)
route.commit()
- spicedb.set_http_url(f"http://{route.hostname}")
+ spicedb.set_http_url(f"http://{spicedb.service.external_ip}")
return routeThis is just a quick hack to check if it is working, I would expect the client to be created in the SpiceDB class.
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.
@averevki why did you recommend using the Openshift route?
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.
Using OpenShift Route instead of LoadBalancer due to stability/connection issues with LoadBalancer services when accessing SpiceDB. Added skip for Kind clusters.
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
1a55de4 to
11bf201
Compare
Description
Changes
Refactoring
testsuite/spicedb/spicedb.py: Complete refactor of SpiceDB integrationSpiceDBServicewithSpiceDB(Backend) andSpiceDBClientclasseshttpxfor SpiceDB HTTP API (/v1/schema/write,/v1/relationships/write,/v1/permissions/check)Configuration
config/settings.yaml: Addedspicedb.imageconfiguration with default imagequay.io/authzed/spicedb:latestconfig/settings.local.yaml.tpl: Updated template to useimageinstead ofurlandpasswordtestsuite/config/__init__.py: Removed validators forspicedb.urlandspicedb.passwordas they are no longer neededVerification steps
Follow-up
After merging, step 2 from #770 needs to be completed: Remove the SpiceDB instance from tools.