Allow Porter Operational timeout parameters for /decrypt
#53
Conversation
derekpierre
changed the title
/decrypt and /reencrypt/decrypt
derekpierre
changed the title
/decrypt/decrypt
derekpierre
requested review from
jMyles,
KPrasch,
cygnusv,
theref,
vzotova,
manumonti and
piotr-roslaniec
|
Looks good in general, but I'm wondering about potential DoS vectors if we allow timeouts to be defined as input parameters. |
Great point - we could put a max cap on the timeout? Perhaps that cap can have a default value, but be overridden by an env variable? That way the Porter operator can make adjustments to the timeout ... open to other ideas ... Is that something we want to apply to other endpoints eg. The biggest concern is us simply hardcoding a timeout in the code that is potentially insufficient. |
Configuration via environment variables or some config file are good. We wouldn't want to make a release just to increment a default timeout. |
derekpierre
changed the title
/decrypt/decrypt
… and python API).
…lue (default that can be modified via an environment variable).
…timeout. Ensure that timeout for decryption is always passed to ThresholdDecryptionClient.
derekpierre
force-pushed
the
node-timeout
branch
from
386612f
to
ce5af4e
Compare
…_TIMEOUT so that it always matches the default timeout used in `nucypher`.
derekpierre
changed the title
/decrypt/decrypt
Pipfile
Outdated
| nucypher-core = "==0.13.0" # must be the same as nucypher | ||
| flask-cors = "*" | ||
| prometheus-flask-exporter = "*" | ||
|
|
||
| [dev-packages] | ||
| nucypher = {git = "https://github.com/nucypher/nucypher.git", editable = true, ref = "v7.0.0-rc.10", extras = ["dev"]} # needed for testerchain, and must be editable | ||
| nucypher = {git = "https://github.com/nucypher/nucypher.git", editable = true, ref = "v7.0.0", extras = ["dev"]} # needed for testerchain, and must be editable |
There was a problem hiding this comment.
Should these values be updated to latest version (v7.0.3)?
| DEFAULT_PORT = 9155 | ||
|
|
||
| MAX_GET_URSULAS_TIMEOUT = os.getenv("PORTER_GET_URSULAS_TIMEOUT", default=15) | ||
| MAX_DECRYPTION_TIMEOUT = os.getenv( | ||
| "PORTER_MAX_DECRYPTION_TIMEOUT", |
There was a problem hiding this comment.
Can envvars be documented somewhere?
There was a problem hiding this comment.
👍 I was thinking the threshold docs, and the repos readme.
Those will be done separately.
| value_factory=value_factory, | ||
| target_successes=quantity, | ||
| timeout=timeout, | ||
| stagger_timeout=1, |
There was a problem hiding this comment.
This is the timeout between obtaining batches of values for processing - https://github.com/nucypher/nucypher/blob/main/nucypher/utilities/concurrency.py#L322.
In this case batches of staking provider addresses.
| if timeout and timeout > self.MAX_GET_URSULAS_TIMEOUT: | ||
| self.log.warn( | ||
| f"Provided sampling timeout ({timeout}s) exceeds " | ||
| f"maximum ({self.MAX_GET_URSULAS_TIMEOUT}s); " | ||
| f"using {self.MAX_GET_URSULAS_TIMEOUT}s instead" | ||
| ) | ||
| timeout = self.MAX_GET_URSULAS_TIMEOUT | ||
| else: | ||
| timeout = timeout or self.MAX_GET_URSULAS_TIMEOUT |
There was a problem hiding this comment.
Not a blocker, but something we can address in the future: if I understand well, we are not checking in the tests that if the provided timeout > MAX_GET_URSULAS_TIMEOUT, the timeout must be MAX_GET_URSULAS_TIMEOUT.
There was a problem hiding this comment.
You're right. I'll look into testing that separately. #56
Type of PR:
Required reviews:
What this does:
Allow Porter to accept optional timeout values for performing TACo decrypt operation.
While this will be provided, In order for web apps to actually use it,
nucypher-ts/taco-apiwill need to be updated to allow timeout to optionally specified.Issues fixed/closed:
Fixes #49 .
Depends on
nucypherPR - nucypher/nucypher#3337Follow-up for
/reencryptfiled in #54 .Why it's needed:
Notes for reviewers: