Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
| """ | ||
| is_s3 = service == _SERVICE_S3 | ||
| not_forwarded = ( | ||
| _HEADERS_NOT_FORWARDED_TO_S3 if is_s3 else _HEADERS_NOT_FORWARDED |
There was a problem hiding this comment.
This is a bit redundant since _HEADERS_NOT_FORWARDED_TO_S3 is just _HEADERS_NOT_FORWARDED except "authorization", and the "authorization" header is overwritten anyway a couple lines later if "not is_s3".
Could we just remove "authorization" from _HEADERS_NOT_FORWARDED and get rid of _HEADERS_NOT_FORWARDED_TO_S3?
There was a problem hiding this comment.
yeah, good call. I cleaned it up and added a check b/c of case-sensitivity causing duplicates
| else: | ||
| logging.warning( | ||
| f"Refusing to proxy {path}: only /ga4gh/tes and /s3 paths are " | ||
| "proxied. Check the endpoints your pipeline is configured with." |
There was a problem hiding this comment.
The paths start with either /ga4gh/tes/v1 or /workflows, see config here.
Plus, the S3 endpoint is also exposed at the app root (see here).
So a request to Gen3 S3 could start with /workflows or /workflows/s3 or /ga4gh/tes/v1 or /ga4gh/tes/v1/s3 (although the last 2 are not documented or used). But never just /s3.
I'm not sure what a reliable way to identify S3 requests while maintaining root S3 endpoint support would be. We could list all the non-S3 routes but that's not very future-proof.
For now to unblock my testing, i made this change, which assumes S3 requests are non-root.
Edit: I see the proxy is accepting /s3 requests and forwarding them to /workflows/s3, and DPOP_PROTECTED_PATHS in gen3-workflow matches that, so maybe I misunderstood the intent. We can discuss it when you're back!
There was a problem hiding this comment.
the proxy itself has its own endpoints and then translates those to the real Gen3 Workflow, and I'd rather keep them separate and have the mapping in the proxy itself. We can control in the Nextflow config what local proxy endpoint to hit for S3 and TES respectively (and the auto-generated config should already be doing that). e.g. this should've worked out of the box
in your edit: /ga4gh/tes and /s3 are the proxy's own local namespace, not commons paths. the commons paths only appear in TES_ENDPOINT / S3_ENDPOINT, and the generated nextflow config points nextflow at the local ones.
round trip for S3: nextflow hits 127.0.0.1:port/s3/bucket/key, proxy strips /s3 and appends to {commons}/workflows/s3, signs htu over {commons}/workflows/s3/bucket/key.
gen3-workflow should rebuild that same string to check.
What was the config that sent /workflows/... at the proxy? e.g. why didn't what was written work out of the box?
if it was a hand-written pipeline config or the auto-generated config... we could fix that instead.
re: your changes, if we want to keep this and support commons paths matching locally - I think it needs some updates either way. It looks like this will happen:
/workflows/s3/bucket/key.txt -> ('https://cx/workflows/s3/bucket/key.txt', 's3')
/workflows/bucket/key.txt -> ('https://cx/ga4gh/tes/bucket/key.txt', 'tes')
/s3/bucket/key.txt -> None
the middle one is the root-mounted S3 case you raised - after /workflows/ matches, anything that isn't /s3/ falls through to the TES base, so it goes to
{commons}/ga4gh/tes/<bucket>/<key> and since the service isn't s3 we replace the
SigV4 header with Authorization: DPoP <token>. and the last one is what the
generated config sends, so generated-config runs 404.
But... I still am not convinced we need to change this. The local proxy and nextflow config can be opinionated - we can choose to only support /s3 -> /workflows/s3 instead of all 4 options the service itself supports.
There was a problem hiding this comment.
My integration tests do not use the path that updates the config automatically. I see that I misconfigured aws.client.endpoint (<proxy>/workflows/s3 instead of <proxy>/s3), that's probably where the issue came from. I'll fix that and test - no need to change anything if that works 👍
Maybe a bit of documentation/docstring about this endpoint mapping would be nice though, so the next reader isn't confused like I was?
There was a problem hiding this comment.
Ok that was only part of the issue - all the existing TES tests were also pointing at /workflows/s3, which i didn't change when i updated them to use the dpop proxy.
Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
… match the known env vars for tokens so other processes/users cannot just hit it without also sending auth
nss10
left a comment
nss10
left a comment
There was a problem hiding this comment.
Great work. Left some comments, questions and suggestions.
| # A client acting on behalf of a user appends the user ID to its token. | ||
| return candidate.split(";userId=")[0] or None |
There was a problem hiding this comment.
@pauline -- Do we still need this here? Now that we are moving away from client accessing S3 bucket on users' behalf?
| """ | ||
| Route a path to its upstream base, or refuse to route it at all. | ||
|
|
||
| Only TES and S3 traffic belongs on this proxy, so the two prefixes are |
There was a problem hiding this comment.
The /ga4gh/tes and /s3 prefixes are hardcoded in _resolve_upstream_url and referenced in several comments. If a third service is added later, those will need to be found and updated together. Worth considering whether the route table should be data-driven (e.g. a dict of prefix → service) so adding a service is one change in one place — but fine to defer if the two-service assumption and extensibility is out of scope for now.
There was a problem hiding this comment.
I'd vote to defer for now. a route table would only centralize the prefix matching - each service's endpoint also flows through its own --*-endpoint CLI option, resolve_service_endpoints (default + same-host check), the proxy config, and the generated nextflow config. so a third service needs customization regardless; worth revisiting if one actually shows up (I think it will be more clear at that point what can be consolidated)
| gen3users = "*" | ||
| joserfc = ">=1.7.3" | ||
|
|
||
| authutils = {git = "https://github.com/uc-cdis/authutils.git", rev = "feat/dpop"} |
There was a problem hiding this comment.
Reminder to pin it to master, after authutils PR is merged
| self.records: dict[str, dict] = {} | ||
| self.bundles: dict[str, dict] = {} |
There was a problem hiding this comment.
Should reads and writes to these dicts be thread safe? Since mutliple threads could "technically" write in parallel. I don't think it is an issue with the current use case though.
There was a problem hiding this comment.
agreed it's not an issue today. in-process mode is single threaded, and the threaded HTTP mode only serves download_object_manifest, which reads. if a test ever starts writing concurrently over HTTP, we'd maybe need to revisit but I don't expect we'd need / want a fake indexd server for that (we'd probably just patch at the requests / httpx2 call instead of handling this way). All of this was to remove the weird dependency on indexd for unit tests
| def test_no_requested_lifetime_skips_the_check(self, ec_key, requests_mock): | ||
| """Without an explicit lifetime the server picks one, so nothing is checked.""" | ||
| token, _ = _exchange(ec_key, api_key=_api_key_expiring_in(-60)) | ||
|
|
||
| assert token == TASK_TOKEN | ||
| assert requests_mock.called |
There was a problem hiding this comment.
This seems a little off to me — maybe I'm missing something. There is no restriction on fetching a TASK_TOKEN with an already-expired API key as long as no explicit task_token_expiration is provided?
At dpop.py#L920-921 we simply skip the check when task_token_expiration is None. Is this a missed edge case, or are we intentionally deferring to the server to reject the expired API key?
There was a problem hiding this comment.
great catch, that was a legit missed edge case. fixing
| method, upstream_url, scope.get("headers", []), service | ||
| ) | ||
|
|
||
| with tempfile.SpooledTemporaryFile(max_size=_MAX_BODY_SIZE_IN_MEMORY) as body: |
There was a problem hiding this comment.
I know buffering the body is necessary to support nonce retries, but I'm concerned about the disk space implications for large uploads. If a user uploads a 5 GB file, the proxy needs 5 GB of free space in /tmp for the duration of the transfer. With parallel uploads, that multiplies — a user could unknowingly need tens of gigabytes of temporary storage just to run the proxy.
This seems worth documenting. maybe in docs/howto/nextflow.md ?
There was a problem hiding this comment.
good point, adding a note to the nextflow howto
|
|
||
| _refusal(ec_key) | ||
|
|
||
| assert requests_mock.call_count == 3 |
There was a problem hiding this comment.
Probably 3 is implicitly understood, but can we have _MAX_NONCE_RETRIES + 1 to be cleaner?
There was a problem hiding this comment.
yeah, I was avoiding pulling in private globals/functions by hard-coding it - but I agree with you that it's more readable that way (and since this is just tests, I'm okay bending that "no importing privates" rule). I made this change and the suggested test breakup with testing a private function below too
|
|
||
| assert response.status_code == 401 | ||
| assert response.json() == {"error": "use_dpop_nonce"} | ||
| assert proxy.upstream.nonce_challenges_sent == 3 |
There was a problem hiding this comment.
Same as above --
| assert proxy.upstream.nonce_challenges_sent == 3 | |
| assert proxy.upstream.nonce_challenges_sent == _MAX_NONCE_RETRIES + 1 |
Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>
Co-authored-by: Sai Shanmukha Narumanchi <nss10@outlook.com>
Avantol13
left a comment
Avantol13
left a comment
There was a problem hiding this comment.
thank you for the comprehensive review!! I know this was a lot of new code and changes
|
|
||
| def capture(*args: Any, **kwargs: Any) -> _CompletedProcess: | ||
| argv = args[0] | ||
| captured["path"] = Path(argv[argv.index("-c") + 1]) |
There was a problem hiding this comment.
good catch, applying your suggestion
| def test_challenge_carrying_only_a_nonce_header_is_retried( | ||
| self, ec_key, requests_mock | ||
| ): | ||
| """A refusal with a nonce but no explanation is still read as a nonce demand.""" | ||
| requests_mock.post( | ||
| TOKEN_ENDPOINT, | ||
| [ | ||
| { | ||
| "status_code": 401, | ||
| "text": "", | ||
| "headers": {"DPoP-Nonce": "as-nonce-2"}, | ||
| }, | ||
| {"status_code": 200, "json": {"access_token": TASK_TOKEN}}, | ||
| ], | ||
| ) | ||
|
|
||
| assert _exchange(ec_key) == (TASK_TOKEN, "as-nonce-2") |
There was a problem hiding this comment.
deliberate. per RFC 9449 the server signals with error: use_dpop_nonce, but servers can attach a DPoP-Nonce header to every response on a DPoP endpoint, so the header alone doesn't mean "retry with this nonce". the rule in _is_dpop_nonce_error is: if the body explains the failure, trust the body. only when the body is empty is the header the only signal left, so we treat it as a nonce demand. the unrelated error test has a body saying something else, so retrying would just burn the budget and bury the real error. I'll update the docstrings so this is more clear
| """ | ||
| Route a path to its upstream base, or refuse to route it at all. | ||
|
|
||
| Only TES and S3 traffic belongs on this proxy, so the two prefixes are |
There was a problem hiding this comment.
I'd vote to defer for now. a route table would only centralize the prefix matching - each service's endpoint also flows through its own --*-endpoint CLI option, resolve_service_endpoints (default + same-host check), the proxy config, and the generated nextflow config. so a third service needs customization regardless; worth revisiting if one actually shows up (I think it will be more clear at that point what can be consolidated)
| method, upstream_url, scope.get("headers", []), service | ||
| ) | ||
|
|
||
| with tempfile.SpooledTemporaryFile(max_size=_MAX_BODY_SIZE_IN_MEMORY) as body: |
There was a problem hiding this comment.
good point, adding a note to the nextflow howto
|
|
||
| _refusal(ec_key) | ||
|
|
||
| assert requests_mock.call_count == 3 |
There was a problem hiding this comment.
yeah, I was avoiding pulling in private globals/functions by hard-coding it - but I agree with you that it's more readable that way (and since this is just tests, I'm okay bending that "no importing privates" rule). I made this change and the suggested test breakup with testing a private function below too
| def test_no_requested_lifetime_skips_the_check(self, ec_key, requests_mock): | ||
| """Without an explicit lifetime the server picks one, so nothing is checked.""" | ||
| token, _ = _exchange(ec_key, api_key=_api_key_expiring_in(-60)) | ||
|
|
||
| assert token == TASK_TOKEN | ||
| assert requests_mock.called |
There was a problem hiding this comment.
great catch, that was a legit missed edge case. fixing
| run_cli(["main.nf", "-c", "mine.config"]) | ||
|
|
||
| argv = nextflow_process.call_args.args[0] | ||
| assert argv[:5] == ["nextflow", "run", "main.nf", "-c", "mine.config"] |
There was a problem hiding this comment.
no good reason, will switch it to _pipeline_args for consistency
| self.records: dict[str, dict] = {} | ||
| self.bundles: dict[str, dict] = {} |
There was a problem hiding this comment.
agreed it's not an issue today. in-process mode is single threaded, and the threaded HTTP mode only serves download_object_manifest, which reads. if a test ever starts writing concurrently over HTTP, we'd maybe need to revisit but I don't expect we'd need / want a fake indexd server for that (we'd probably just patch at the requests / httpx2 call instead of handling this way). All of this was to remove the weird dependency on indexd for unit tests
| headers={"DPoP-Nonce": "as-nonce-1"}, | ||
| ) | ||
|
|
||
| _refusal(ec_key) |
There was a problem hiding this comment.
yeah, good call. I'm going to remove it and be explicit
| @pytest.mark.parametrize( | ||
| "body,expected", | ||
| [ | ||
| # Whichever field fence puts its explanation in, it is quoted back. | ||
| pytest.param({"json": {"message": "no can do"}}, "no can do", id="message"), | ||
| pytest.param( | ||
| {"json": {"error_description": "no can do"}}, | ||
| "no can do", | ||
| id="error_description", | ||
| ), | ||
| pytest.param({"json": {"detail": "no can do"}}, "no can do", id="detail"), | ||
| pytest.param({"json": {"error": "no can do"}}, "no can do", id="error"), | ||
| # An HTML or plain-text body is included rather than dropped. | ||
| pytest.param( | ||
| {"text": "<html>bad gateway</html>"}, "bad gateway", id="html" | ||
| ), | ||
| # A JSON error with no field this SDK knows about is reported as-is. | ||
| pytest.param({"json": {"weird": ["shape"]}}, '"weird"', id="unknown_shape"), | ||
| # JSON that is not an object at all still has to survive the trip. | ||
| pytest.param({"json": ["no can do"]}, "no can do", id="json_array"), | ||
| ], | ||
| ) | ||
| def test_server_explanation_is_surfaced( | ||
| self, ec_key, requests_mock, body, expected | ||
| ): | ||
| """Whatever the server said reaches the caller.""" |
There was a problem hiding this comment.
good callout, I split it up a bit more, lmk what you think
Integration Tests
Please find the detailed integration test report here Please find the Github Action logs here |
Failure AnalysisPlease find the detailed test analysis report here |
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
This branch has not been deployed
4 participants
Depends on:
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes