feat: Add OpaClient to wrap auth checks#1541
Conversation
tpoliaw
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
tpoliaw
force-pushed
the
opa-client
branch
2 times, most recently
from
e8ddc77 to
db3325b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## depends-user #1541 +/- ##
================================================
+ Coverage 95.63% 95.77% +0.14%
================================================
Files 43 44 +1
Lines 3228 3264 +36
================================================
+ Hits 3087 3126 +39
+ Misses 141 138 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| class OpaConfig(BlueapiBaseModel): | ||
| root: HttpUrl = HttpUrl("http://localhost:8181") |
There was a problem hiding this comment.
why hard coded this URL in ? will this always the case on beamline?
There was a problem hiding this comment.
The hardcoded value is a default (the opa server used in system tests) similar to the ones for numtracker and OIDC. On beamlines, this will be overwritten by the config (in the beamline's values.yaml)
There was a problem hiding this comment.
Thanks for the explaination as from the codes I did see how this URL being replaced by other on beamline, I must have missed some codes that does the overwrite.
| } | ||
| }, | ||
| "additionalProperties": false | ||
| }, |
There was a problem hiding this comment.
I am new to cloud programming, but would like to understand why there are the same json section here as in config_schema.json above?
There was a problem hiding this comment.
When deploying blueapi via helm, the config is included in the values.yaml file. When the schema of the config changes, the embedded config in the values file changes in the same way.
| def __init__(self, instrument: str, config: OpaConfig): | ||
| LOGGER.info("Creating OpaClient for %s with config %s", instrument, config) | ||
| self._instrument = instrument | ||
| self._conf = config |
There was a problem hiding this comment.
nit: it is a private variable so you can leave it as it is
| self._conf = config | |
| self._config = config |
| json={ | ||
| "input": { | ||
| "beamline": self._instrument, | ||
| "audience": "account", |
There was a problem hiding this comment.
I have been going back and forth on this, but I think this should be configurable in the config rather than hardcoded. we can default it to "account"
| "audience": "account", | |
| "audience": self._audience, |
| LOGGER.info("Closing OPA session") | ||
| await self._session.close() | ||
|
|
||
| async def _call_opa(self, endpoint, data: Mapping[str, Any]) -> bool: |
There was a problem hiding this comment.
| async def _call_opa(self, endpoint, data: Mapping[str, Any]) -> bool: | |
| async def _call_opa(self, endpoint: str, data: Mapping[str, Any]) -> bool: |
| } | ||
| }, | ||
| ) | ||
| return (await resp.json())["result"] |
There was a problem hiding this comment.
nit: Will be good to TypeAdapter the result to bool because of python
Something like this
from pydantic import TypeAdapter
return TypeAdapter(bool).validate_python((await resp.json())["result"])
I think we should as put this in
try:
TypeAdapter(bool).validate_python((await resp.json())["result"])
except KeyError : # on result unlikely
...
except Timeout to OPA as e:
raise e as Timeouterror
or just Exception
There was a problem hiding this comment.
What does the TypeAdapter protect against? OPA returning "False" instead of False?
For the exception handling, does wrapping the exception here add much beyond letting the original exception be raised?
3 participants
Proof of concept opa client with dependency injection and example check