SDK | Update aws sts cred method #9218
Conversation
WalkthroughReplaces AWS STS credential and STS-S3 client helper calls with AWS SDK v3 equivalents across S3-related paths. Updated functions: generate_aws_sdkv3_sts_creds and createSTSS3SDKv3Client. Logic, parameters, branching, and error handling remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller (BlockStore/Namespace/Servers)
participant CU as cloud_utils
participant STS as AWS STS
participant S3 as AWS S3 Client (SDK v3)
Caller->>CU: generate_aws_sdkv3_sts_creds(params, scope)
CU->>STS: AssumeRole / GetSessionToken
STS-->>CU: Temporary creds (AK/SK/SessionToken)
CU-->>Caller: STS creds
Caller->>S3: create client (v3) with creds
S3-->>Caller: Client instance
note over Caller,S3: Subsequent S3 ops proceed as before
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (3)
src/agent/block_store_services/block_store_client.js (2)
306-310: Avoid logging secrets (omit more keys).Current omit only hides
secretAccessKey. STS/non-STS paths may exposesecret_keyandsessionToken.- dbg.log1('_delegate_write_block_s3:', - 'got s3_params from block_store. writing using S3 sdk. s3_params =', - _.omit(s3_params, 'secretAccessKey'), + dbg.log1('_delegate_write_block_s3:', + 'got s3_params from block_store. writing using S3 sdk. s3_params =', + _.omit(s3_params, ['secretAccessKey','secret_key','sessionToken']), 'aws_sts_arn', bs_info.connection_params.aws_sts_arn);
359-363: Avoid logging secrets on read path.Mirror the logging scrub.
- dbg.log1('_delegate_read_block_s3:', - 'got s3_params from block_store. writing using S3 sdk. s3_params =', - _.omit(s3_params, 'secretAccessKey'), + dbg.log1('_delegate_read_block_s3:', + 'got s3_params from block_store. writing using S3 sdk. s3_params =', + _.omit(s3_params, ['secretAccessKey','secret_key','sessionToken']), 'aws_sts_arn', bs_info.connection_params.aws_sts_arn);src/sdk/namespace_s3.js (1)
69-76: Action required — STS S3 client is method-compatible but does NOT preserve agent/endpoint/auto-region; fix needed.createSTSS3SDKv3Client returns a v3 S3 instance (supports listObjects/getObject and .send) and sets region from params, but it does not reuse the original client's requestHandler/HTTP agent, endpoint, or the S3ClientAutoRegion send-wrapper. NamespaceS3._prepare_sts_client calls it with only RoleSessionName, so custom agent/endpoint/auto-region behavior from the original this.s3 will be lost.
- Location: src/util/cloud_utils.js:createSTSS3SDKv3Client — currently builds new S3({ credentials, region, endpoint: additionalParams.endpoint, requestHandler: new NodeHttpHandler({ httpsAgent: additionalParams.httpOptions }), ... }).
- Caller: src/sdk/namespace_s3.js:_prepare_sts_client — passes only RoleSessionName (no endpoint/agent), overwriting the previous this.s3 created by noobaa_s3_client.get_s3_client_v3_params(...).
Recommended fixes (choose one):
- Pass endpoint and the underlying agent into additionalParams when calling createSTSS3SDKv3Client (e.g., additionalParams.endpoint = this.s3_params.endpoint and additionalParams.httpOptions = http_utils.get_agent_by_endpoint(this.s3_params.endpoint)) so requestHandler/agent and forcePathStyle are preserved.
- Or modify createSTSS3SDKv3Client to reuse params.requestHandler / params.httpOptions (or construct requestHandler from params.endpoint) and/or return/wrap the client with the existing S3ClientAutoRegion wrapper (see src/sdk/noobaa_s3_client/noobaa_s3_client_sdkv3.js) so the send() auto-region behavior is preserved.
Losing agent/endpoint or the auto-region send override is a behavioral regression and must be addressed.
🧹 Nitpick comments (3)
src/sdk/namespace_s3.js (1)
71-73: Nit: clearer session name for auditing.Use a RoleSessionName that reflects this component.
- const additionalParams = { - RoleSessionName: 'block_store_operations', - }; + const additionalParams = { + RoleSessionName: 'namespace_s3_operations', + };src/agent/block_store_services/block_store_client.js (2)
300-305: STS v3 creds: tolerate both key casings.Prevent runtime auth issues if the helper returns capitalized fields.
- if (bs_info.connection_params.aws_sts_arn) { - const creds = await cloud_utils.generate_aws_sdkv3_sts_creds(s3_params, "_delegate_write_block_s3_session"); - s3_params.accessKeyId = creds.accessKeyId; - s3_params.secretAccessKey = creds.secretAccessKey; - s3_params.sessionToken = creds.sessionToken; - } + if (bs_info.connection_params.aws_sts_arn) { + const creds = await cloud_utils.generate_aws_sdkv3_sts_creds(s3_params, "_delegate_write_block_s3_session"); + s3_params.accessKeyId = creds.accessKeyId || creds.AccessKeyId; + s3_params.secretAccessKey = creds.secretAccessKey || creds.SecretAccessKey; + s3_params.sessionToken = creds.sessionToken || creds.SessionToken; + }
354-358: STS v3 creds: casing tolerance on read path as well.Match the write-path fix.
- if (bs_info.connection_params.aws_sts_arn) { - const creds = await cloud_utils.generate_aws_sdkv3_sts_creds(s3_params, "_delegate_read_block_s3_session"); - s3_params.accessKeyId = creds.accessKeyId; - s3_params.secretAccessKey = creds.secretAccessKey; - s3_params.sessionToken = creds.sessionToken; - } + if (bs_info.connection_params.aws_sts_arn) { + const creds = await cloud_utils.generate_aws_sdkv3_sts_creds(s3_params, "_delegate_read_block_s3_session"); + s3_params.accessKeyId = creds.accessKeyId || creds.AccessKeyId; + s3_params.secretAccessKey = creds.secretAccessKey || creds.SecretAccessKey; + s3_params.sessionToken = creds.sessionToken || creds.SessionToken; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/agent/block_store_services/block_store_client.js(2 hunks)src/sdk/namespace_s3.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/server/system_services/bucket_server.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/server/system_services/account_server.js (1)
src/server/system_services/bucket_server.js (2)
creds(2040-2040)cloud_utils(25-25)
src/server/system_services/bucket_server.js (1)
src/server/system_services/account_server.js (2)
creds(1054-1054)cloud_utils(16-16)
src/agent/block_store_services/block_store_client.js (2)
src/server/system_services/account_server.js (3)
creds(1054-1054)cloud_utils(16-16)s3_params(1064-1074)src/server/system_services/bucket_server.js (3)
creds(2040-2040)cloud_utils(25-25)s3_params(2048-2058)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (2)
src/server/system_services/bucket_server.js (1)
2036-2044: No change needed — helper already returns lower-cased STS fields.
generate_aws_sdkv3_sts_creds maps response.Credentials.AccessKeyId/SecretAccessKey/SessionToken to accessKeyId/secretAccessKey/sessionToken (see src/util/cloud_utils.js) and callers/tests use the lower-cased keys.Likely an incorrect or invalid review comment.
src/server/system_services/account_server.js (1)
1053-1058: No change required — helper returns lowerCamel STS creds.
generate_aws_sdkv3_sts_creds (src/util/cloud_utils.js) returns { accessKeyId, secretAccessKey, sessionToken }, so the current assignments are correct and the capitalization guard is unnecessary.Likely an incorrect or invalid review comment.
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
s3_sdk_v3
branch
from
4440839 to
64ffae8
Compare
2 participants
Describe the Problem
account_server and bucket_server still using old AWS s2 sts cred generating code
Explain the Changes
generate_aws_sts_creds()withgenerate_aws_sdkv3_sts_creds()Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit