-
Notifications
You must be signed in to change notification settings - Fork 61
HARMONY-2240: Fix to make sure custom query parameters are added to S3 URLs again #840
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
Conversation
…3 URLs again. Deal with latest round of audit failures.
📝 WalkthroughWalkthroughReplaced/removed several Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Harmony as Harmony:ObjectStore
participant S3 as S3 Service
participant Presigner as S3RequestPresigner
Note over Client,Harmony: signGetObject(bucket,key,params)
Client->>Harmony: signGetObject(bucket,key,params)
Harmony->>S3: HeadObjectCommand(Bucket,Key)
S3-->>Harmony: HeadObject OK / Error
alt HeadObject OK
Harmony->>S3: GetObjectCommand(Bucket,Key) (to get base signed url)
S3-->>Harmony: Signed URL (base + query)
Harmony->>Harmony: parse base URL, attach provided params
Harmony->>Presigner: build HttpRequest + presign (sha256)
Presigner-->>Harmony: presigned URL
Harmony-->>Client: formatted presigned URL (apply localstack fix if needed)
else HeadObject Error
Harmony-->>Client: propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…igher vulnerabilities and clean up .nsprc outdated entries.
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/service-runner/package.json (1)
29-45: Remove or update stale S3 client locked dependency comment.The comment on line 35 states the library was "downgraded due to performance issues," but the current version
^3.972.0(line 38) is actually higher than the previously pinned version3.437.0. Additionally, commit73604f1c(HARMONY-2232) subsequently updated AWS SDK libraries to address open telemetry-related performance issues, suggesting the original downgrade rationale is no longer applicable. Either remove the comment or update it to reflect the current version and context.
…tests due to stubs not being restored correctly.
Jira Issue ID
HARMONY-2240
Description
There was a regression causing custom query parameters to not be included in pre-signed S3 URLs. This PR fixes that and deals with the latest round of audit failures as well.
Local Test Steps
Install dependencies and build all the services. I tested with harmony in a box and in a sandbox environment.
Verify that all microservices come up in k8s successfully (with no errors about missing dependencies).
Verify in an AWS environment that pulling back job results has the query parameters included and that the file is returned (not a 403 error).
e.g. from a
curl -v -Ln -bj --socks5-hostname localhost:8080 "<root>/service-results/..."you can see the URL includes the custom query parameters:PR Acceptance Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.