Use @aws-sdk/credential-providers for smarter credential discovery that works with instance metadata

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d6ab5eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Pull Request Review: AWS Bedrock Credential Provider Enhancement

Summary

This PR improves AWS credential discovery for Bedrock by adding @aws-sdk/credential-providers to use the Node.js provider chain. This enables automatic credential resolution from multiple sources including IAM roles, environment variables, and shared credentials files.

---

✅ Code Quality & Best Practices

Strengths:

• Clean API design: The credentialProvider parameter is properly optional with a sensible default
• Excellent documentation: The JSDoc comment clearly explains the credential chain behavior and use cases (packages/forms/src/llm/providers/bedrock.ts:12-19)
• Backward compatible: Existing code will continue to work without changes
• Type-safe: Proper use of TypeScript types from @aws-sdk/types
• Minimal changes: Focused PR that does exactly what it needs to without over-engineering

Suggestions:

1. Default export location: The default value fromNodeProviderChain() is created on every function call (bedrock.ts:48). Consider whether this should be a singleton to avoid creating multiple credential provider chains:

   const DEFAULT_CREDENTIAL_PROVIDER = fromNodeProviderChain();
   
   export const createBedrockModel = (
     config: Partial<BedrockConfig> = {}
   ): LanguageModel => {
     const {
       modelId,
       region,
       credentialProvider = DEFAULT_CREDENTIAL_PROVIDER,
     } = { ...DEFAULT_BEDROCK_CONFIG, ...config };
     // ...
   };

   However, this is a minor point and the current approach is acceptable.

---

🐛 Potential Bugs or Issues

No critical issues identified. The implementation is straightforward and safe.

Minor consideration:

• If fromNodeProviderChain() throws during initialization, the error will be thrown at function call time rather than module load time. This is actually preferable for testability, so current implementation is good.

---

⚡ Performance Considerations

Positive:

• The credential provider chain is lazy-loaded and cached by the AWS SDK internally
• No performance regressions expected

Neutral:

• Creating fromNodeProviderChain() on each call has minimal overhead (it's just instantiation, not actual credential resolution)
• If this becomes a hot path and profiling shows issues, consider the singleton approach mentioned above

---

🔒 Security Concerns

Positive changes:

• ✅ Follows AWS security best practices by using the standard credential chain
• ✅ Enables IAM role-based authentication (crucial for production deployments)
• ✅ Reduces reliance on long-lived access keys in environment variables
• ✅ Supports instance metadata for EC2/ECS/App Runner, which is the most secure approach

Recommendations:

1. Documentation: Consider adding a security note in the codebase about credential best practices (prefer IAM roles > temporary credentials > long-lived keys)
2. Environment detection: The code works well, but operational documentation should clarify which credential methods are preferred in each deployment environment

---

🧪 Test Coverage

Concern: No tests found for the Bedrock provider module.

Recommendations:

1. Add unit tests for createBedrockModel():

   • Test default configuration
   • Test custom configuration override
   • Test custom credential provider injection
   • Mock the createAmazonBedrock call to verify correct parameters

2. Example test structure:

   describe('createBedrockModel', () => {
     it('should use default configuration', () => {
       const model = createBedrockModel();
       // Assert model creation with defaults
     });
     
     it('should merge custom config with defaults', () => {
       const model = createBedrockModel({ region: 'us-west-2' });
       // Assert custom region is used
     });
     
     it('should accept custom credential provider', () => {
       const customProvider = async () => ({ /* credentials */ });
       const model = createBedrockModel({ credentialProvider: customProvider });
       // Assert custom provider is used
     });
   });

---

📦 Dependency Analysis

Packages added:

• @aws-sdk/credential-providers@^3.901.0 - 🟢 Well-maintained AWS official package
• @aws-sdk/types@^3.901.0 - 🟢 Type definitions from AWS SDK

Observations:

• Both packages are from the official AWS SDK v3
• Version 3.901.0 is recent (November 2024)
• No security vulnerabilities expected (AWS SDK is actively maintained)
• Large lockfile change (1000+ lines) is expected with AWS SDK dependencies but appears normal

Recommendation:

• Consider using a version range like ^3.900.0 to allow patch updates, but the current ^3.901.0 is fine

---

📋 Overall Assessment

Rating: ✅ Approve with minor suggestions

This is a solid, well-designed PR that improves AWS credential handling for production deployments. The code is clean, well-documented, and follows AWS best practices.

Before merging:

1. ✅ Code changes are good to go
2. ⚠️ Consider adding unit tests for the Bedrock provider
3. ⚠️ Update operational documentation about credential configuration in different environments

Priority: The lack of tests is not a blocker if this code is already being tested indirectly through integration tests (e.g., via BedrockParser), but adding focused unit tests would improve maintainability.

Great work! 🎉