AWS config updates #777
AWS config updates #777
Conversation
kjamrog
requested review from
a team,
CarlosGamero,
dariacm and
kibertoad
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Base Resolver Abstraction packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts |
Added protected abstract resolveKmsKeyId(awsConfig: AwsConfig): string and replaced direct awsConfig.kmsKeyId usage with calls to this method in DLQ and main queue/topic attribute construction |
SNS Resolver Implementation packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts |
Added protected override resolveKmsKeyId(awsConfig: AwsConfig) that returns awsConfig.kmsKeyId or falls back to alias/aws/sns when useDefaultKmsKeyId is enabled; replaced direct KMS id usage in topic attributes |
SNS Resolver Tests packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsResolver.spec.ts |
Added tests for useDefaultKmsKeyId behavior verifying default alias/aws/sns is used when awsConfig.kmsKeyId is empty and the flag is enabled |
SQS Resolver Implementation packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts |
Added protected override resolveKmsKeyId(awsConfig: AwsConfig) that returns awsConfig.kmsKeyId or alias/aws/sqs when useDefaultKmsKeyId is enabled; replaced direct KMS id usage in queue attributes |
SQS Resolver Tests packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsResolver.spec.ts |
Added tests covering default KMS alias behavior for internal/external queues when useDefaultKmsKeyId is enabled or disabled |
Configuration Types packages/app/aws-config/src/message-queue-toolkit/types.ts |
Added optional useDefaultKmsKeyId?: boolean to MessageQueueToolkitOptionsResolverConfig |
Topic Naming Regex & Tests packages/app/aws-config/src/message-queue-toolkit/utils.ts, .../utils.spec.ts |
Relaxed TOPIC_NAME_REGEX from /^[a-z]+(_[a-z]+)*-[a-z]+(_[a-z]+)*$/ to /^[a-z]+([_-][a-z]+)+$/ and updated tests to accept additional patterns (allows mixed hyphens/underscores) |
Docs packages/app/aws-config/README.md |
Documented new constructor params validateNamePatterns and useDefaultKmsKeyId in resolver constructor params section |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Caller
participant BaseResolver as AbstractResolver
participant SNSResolver
participant SQSResolver
participant AWSConfig as AwsConfig
Note over Caller,BaseResolver: Queue/Topic resolution flow (high-level)
Caller->>BaseResolver: resolvePublisherOptions(params)
BaseResolver->>AWSConfig: read awsConfig
BaseResolver->>SNSResolver: this.resolveKmsKeyId(awsConfig)
SNSResolver-->>BaseResolver: kmsKeyId (explicit or alias/aws/sns)
BaseResolver->>Caller: returns attributes with KmsMasterKeyId
Caller->>BaseResolver: resolveConsumerOptions(params)
BaseResolver->>SQSResolver: this.resolveKmsKeyId(awsConfig)
SQSResolver-->>BaseResolver: kmsKeyId (explicit or alias/aws/sqs)
BaseResolver->>Caller: returns queue attributes with KmsMasterKeyId
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Reviewers should focus on:
- Correct handling of empty/undefined
awsConfig.kmsKeyIdanduseDefaultKmsKeyIdsemantics in both SNS and SQS overrides. - Proper propagation of
useDefaultKmsKeyIdthrough resolver config and constructor usage. - Tests covering both enabled and disabled states for default KMS behavior and regex changes to ensure no unintended allowed names.
- Correct handling of empty/undefined
Pre-merge checks and finishing touches
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title 'AWS config updates' is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes made. | Use a more specific title that highlights the main changes, such as 'Relax topic naming validation and add default KMS key option' or 'Allow flexible topic names and optional default KMS key handling'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description check | ✅ Passed | The description covers the main changes and includes a partially completed checklist; however, the documentation update box is unchecked despite documentation changes being made. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/aws-config-updates
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsResolver.spec.ts (1)
355-410: Good coverage of SNS default KMS key resolutionThe new tests using
kmsKeyId: ''withuseDefaultKmsKeyId: trueexercise the SNS default-key path for both publisher and consumer flows (including DLQ), and the snapshots correctly assertalias/aws/sns.If not covered elsewhere in this suite, consider adding a small SNS test with
useDefaultKmsKeyId: false+ emptykmsKeyIdto mirror the SQS tests and lock in the “no default” behavior as well.Also applies to: 732-843
packages/app/aws-config/src/message-queue-toolkit/utils.ts (1)
11-16: Regex implementation matches intent; update high-level pattern docThe new
TOPIC_NAME_REGEX = /^[a-z]+([_-][a-z]+)+$/correctly supports lower‑case topic names with one or more-/_‑separated segments (e.g.repo-name-process_name) and still enforces a letter at both ends.The explanatory lines (11–14) are accurate, but the earlier line:
* pattern: <system_name>-<(flow|model)_name>now understates what’s allowed. Consider updating that pattern description (or removing it) so it doesn’t suggest only two segments with a single hyphen.
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts(4 hunks)packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts(3 hunks)packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsResolver.spec.ts(3 hunks)packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts(2 hunks)packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsResolver.spec.ts(3 hunks)packages/app/aws-config/src/message-queue-toolkit/types.ts(1 hunks)packages/app/aws-config/src/message-queue-toolkit/utils.spec.ts(1 hunks)packages/app/aws-config/src/message-queue-toolkit/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts (1)
packages/app/aws-config/src/index.ts (1)
AwsConfig(2-2)
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsResolver.spec.ts (1)
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts (1)
MessageQueueToolkitSqsOptionsResolver(23-101)
packages/app/aws-config/src/message-queue-toolkit/utils.spec.ts (1)
packages/app/aws-config/src/message-queue-toolkit/utils.ts (1)
TOPIC_NAME_REGEX(16-16)
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts (1)
packages/app/aws-config/src/awsConfig.ts (1)
AwsConfig(17-30)
⏰ 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 (22.x)
- GitHub Check: build (24.x)
- GitHub Check: build (20.x)
🔇 Additional comments (5)
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsResolver.spec.ts (1)
51-58: SNS resolver fixture correctly enables default-KMS behaviorConfiguring the shared SNS resolver with
useDefaultKmsKeyId: trueat construction time is appropriate and matches the new KMS resolution flow.packages/app/aws-config/src/message-queue-toolkit/utils.spec.ts (1)
29-34: TOPIC_NAME_REGEX tests align with relaxed naming rulesAllowing
'repo-name-process_name'as a valid topic name matches the new regex semantics for mixed hyphen/underscore separators and multiple segments; the positive/negative sets still look coherent.packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts (1)
9-10: Abstract KMS resolution hook is well-factoredPulling KMS key resolution into
resolveKmsKeyIdand using it consistently for both main and DLQ queue attributes is a clean abstraction and keeps default-key policy in concrete resolvers instead of scattering checks here.Also applies to: 49-50, 119-120, 157-158
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts (1)
9-9: SQS KMS default resolution logic looks correctThe override correctly keeps existing behavior when
useDefaultKmsKeyIdis false or a non-emptykmsKeyIdis provided, and otherwise falls back to'alias/aws/sqs'. This matches the new configuration option’s intent without breaking current callers.Also applies to: 83-89
packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsResolver.spec.ts (1)
31-38: SQS tests correctly cover publisher default / non-default KMS key pathsThe fixture configuration with
useDefaultKmsKeyId: trueplus the two internal-queue publisher tests (kmsKeyId: ''anduseDefaultKmsKeyId: false) gives good coverage for both default-alias (alias/aws/sqs) and “no default” behavior.Also applies to: 239-339
| import type { AwsConfig } from '../awsConfig.ts' | ||
| import type { EventRoutingConfig, TopicConfig } from '../event-routing/eventRoutingConfig.ts' |
There was a problem hiding this comment.
🧩 Analysis chain
Double-check default KMS alias for SQS queues created by the SNS resolver
Using resolveKmsKeyId to default to 'alias/aws/sns' makes sense for SNS topics themselves, and the wiring into resolveTopic is clean. However, this same method is also used by the shared base class when constructing SQS queue and DLQ attributes in SNS consumer flows, so those queues will also receive KmsMasterKeyId: 'alias/aws/sns' when kmsKeyId === '' and useDefaultKmsKeyId is true.
Given that AWS provides service-managed keys like alias/aws/sns and alias/aws/sqs, it’s worth verifying whether SQS queues should instead default to alias/aws/sqs while SNS topics use alias/aws/sns. If service-managed keys are indeed service-specific, you may want to differentiate topic vs queue defaults (e.g., via separate resolution paths or a more granular helper) to avoid potential KMS permission issues.
Also applies to: 180-186, 206-211
I'll verify the KMS key-alias concern by examining the codebase and AWS documentation.
🏁 Script executed:
#!/bin/bash
# Find and examine the SNS resolver file and related base class
fd -t f "MessageQueueToolkitSnsOptionsResolver" -x cat {}Length of output: 7704
🏁 Script executed:
#!/bin/bash
# Find the base resolver class
fd -t f "AbstractMessageQueueToolkitOptionsResolver" -x cat {}Length of output: 5658
🏁 Script executed:
#!/bin/bash
# Search for resolveKmsKeyId method implementations across the codebase
rg -n "resolveKmsKeyId" --type ts -C 5Length of output: 8518
🏁 Script executed:
#!/bin/bash
# Look for KMS-related tests in SNS resolver tests
fd -t f "*SnsOptionsResolver*.test" -o "*SnsOptionsResolver*.spec" -x cat {}Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for usages of 'alias/aws/sns' and 'alias/aws/sqs' in codebase
rg -n "alias/aws/(sns|sqs)" --type tsLength of output: 974
🏁 Script executed:
#!/bin/bash
# Find test files for the SNS resolver
fd -t f -g "*test*" -g "*spec*" | grep -i "sns.*resolver"Length of output: 93
🏁 Script executed:
#!/bin/bash
# Examine directory structure
find packages/app/aws-config/src/message-queue-toolkit -type f -name "*.ts" | head -20Length of output: 956
🏁 Script executed:
#!/bin/bash
# Check for SQS queue creation in SNS consumer path
rg -n "resolveDlq|resolveSqs|KmsMasterKeyId" --type ts -C 3 | head -100Length of output: 11675
🏁 Script executed:
#!/bin/bash
# Look at imports and class hierarchy
rg -n "extends AbstractMessageQueueToolkitOptionsResolver" --type ts -C 2Length of output: 1335
🏁 Script executed:
#!/bin/bash
# Find where resolveKmsKeyId is called for queue attributes
rg -n "resolveKmsKeyId\s*\(" --type ts -B 2 -A 2Length of output: 4174
🏁 Script executed:
#!/bin/bash
# Search for queue attribute resolution
rg -n "KmsMasterKeyId" --type ts -B 2 -A 2Length of output: 17065
🏁 Script executed:
#!/bin/bash
# Look for test assertions on alias/aws/sns for queues
rg -n "alias/aws/sns.*queue|queue.*alias/aws/sns" --type ts -iLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check SQS resolver for comparison
fd -t f "MessageQueueToolkitSqsOptionsResolver" -x cat {}Length of output: 3482
🏁 Script executed:
#!/bin/bash
# Find all resolver implementations
fd -t f "MessageQueueToolkit.*OptionsResolver\.ts$" | grep -v test | grep -v specLength of output: 277
🏁 Script executed:
#!/bin/bash
# Get the full SNS resolver file content
cat packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 7704
🏁 Script executed:
#!/bin/bash
# Get the base resolver class
cat packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.tsLength of output: 5658
🏁 Script executed:
#!/bin/bash
# Look for SNS consumer specific tests
fd -t f | xargs grep -l "SNS.*consumer\|consumer.*SNS" | grep testLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for where SQS queues are created in SNS context
rg -n "resolveDlq|resolveQueueUrl" --type ts -B 5 -A 5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check AWS documentation patterns in code comments
rg -n "service.managed|alias/aws" --type ts -B 1 -A 1Length of output: 2656
🏁 Script executed:
#!/bin/bash
# List all test files in message queue toolkit directory
find packages/app/aws-config/src/message-queue-toolkit -name "*.test.ts" -o -name "*.spec.ts"Length of output: 342
🏁 Script executed:
#!/bin/bash
# Get SNS resolver tests
find packages/app/aws-config -name "*SnsOptionsResolver*.test.ts" -o -name "*SnsOptionsResolver*.spec.ts" | xargs catLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's an SQS options resolver for comparison
fd -t f "SqsOptionsResolver" -x cat {}Length of output: 3482
🏁 Script executed:
#!/bin/bash
# Search all resolver files to understand the pattern
fd -t f "OptionsResolver\.ts$" | grep -v test | xargs head -50Length of output: 5555
🏁 Script executed:
#!/bin/bash
# Get line numbers and context around lines 180-186 and 206-211
rg -n "." --type ts packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts | grep -E "^(180|181|182|183|184|185|186|206|207|208|209|210|211):"Length of output: 539
🏁 Script executed:
#!/bin/bash
# Look for explicit test cases checking KMS on queues from SNS
rg -n "KmsMasterKeyId.*alias/aws/sns" --type ts -C 5Length of output: 5685
🏁 Script executed:
#!/bin/bash
# Search for test files and examine them more broadly
fd -t f "*resolver*" -path "*message-queue-toolkit*" | grep -E "(test|spec)" | xargs cat 2>/dev/null | head -200Length of output: 168
🏁 Script executed:
#!/bin/bash
# Get complete context of the SNS resolver lines mentioned in review
sed -n '13,14p; 180,186p; 206,211p' packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 635
🏁 Script executed:
#!/bin/bash
# Get broader context around those lines
sed -n '1,300p' packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 7704
🏁 Script executed:
#!/bin/bash
# Check if there is a way to differentiate topic vs queue defaults
rg -n "topic|queue" --type ts packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts -i | head -30Length of output: 1858
🏁 Script executed:
#!/bin/bash
# Look at parent/base class methods
rg -n "resolveTopic|resolveDlq|resolveQueueUrl" --type ts -B 3 -A 10 | head -150Length of output: 5528
🏁 Script executed:
#!/bin/bash
# Find where SQS queue objects get the KMS key
rg -n "attributes.*KmsMasterKeyId\|KmsMasterKeyId.*attributes" --type ts -B 5 -A 5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine imports and class definition of SNS resolver
head -30 packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 1223
🏁 Script executed:
#!/bin/bash
# Look for the resolveTopic method
rg -n "resolveTopic\s*\(" --type ts -A 20Length of output: 5353
🏁 Script executed:
#!/bin/bash
# Look for queue/DLQ resolution via base class in SNS context
rg -n "class.*SnsOptionsResolver|extends" --type ts packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 441
🏁 Script executed:
#!/bin/bash
# Get line count and key sections
wc -l packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 94
🏁 Script executed:
#!/bin/bash
# Full detailed cat of the SNS resolver
cat -n packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.tsLength of output: 9202
🏁 Script executed:
#!/bin/bash
# Look at AbstractMessageQueueToolkitOptionsResolver for queue attribute handling
cat -n packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts | head -100Length of output: 4089
🏁 Script executed:
#!/bin/bash
# Search for where KmsMasterKeyId gets set for queues (not topics)
rg -n "attributes\s*[:=]" --type ts packages/app/aws-config/src/message-queue-toolkit/ -A 5 | grep -A 5 "KmsMasterKeyId"Length of output: 0
🏁 Script executed:
#!/bin/bash
# List all files in the message-queue-toolkit directory
ls -la packages/app/aws-config/src/message-queue-toolkit/Length of output: 936
🏁 Script executed:
#!/bin/bash
# Check tests to see if KMS behavior is tested
find packages/app/aws-config -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "KmsMasterKeyId\|alias/aws" | head -5Length of output: 177
🏁 Script executed:
#!/bin/bash
# Examine a test file
find packages/app/aws-config -type f \( -name "*SnsOptionsResolver*.test.ts" -o -name "*SnsOptionsResolver*.spec.ts" \) -print0 | xargs -0 cat 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# If above didn't work, try broader search
find packages/app -type f -name "*test.ts" -o -name "*spec.ts" | xargs grep -l "SnsOptionsResolver" 2>/dev/null | head -3Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check if there are any test or integration files
fd -t f "test\|spec" packages/app/aws-config/src/message-queue-toolkit/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for __tests__ directory
find packages/app/aws-config -type d -name "__tests__" -o -type d -name "tests" -o -type d -name "test"Length of output: 29
🏁 Script executed:
#!/bin/bash
# More thorough check - all TS files in the area
find packages/app/aws-config/src/message-queue-toolkit -name "*.ts"Length of output: 956
🏁 Script executed:
#!/bin/bash
# Fallback: show the entire SNS resolver with line numbers
if [ -f packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts ]; then
cat -n packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts
else
echo "File not found, searching for similar..."
find . -name "*SnsOptionsResolver*" -type f 2>/dev/null | head -5
fiLength of output: 9202
🏁 Script executed:
#!/bin/bash
# Alternative: show the entire AbstractMessageQueueToolkitOptionsResolver
if [ -f packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts ]; then
cat -n packages/app/aws-config/src/message-queue-toolkit/AbstractMessageQueueToolkitOptionsResolver.ts | head -200
else
echo "File not found"
find . -name "*AbstractMessageQueue*" -type f 2>/dev/null
fiLength of output: 6827
🏁 Script executed:
#!/bin/bash
# Show SQS resolver for comparison if available
if [ -f packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts ]; then
cat -n packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSqsOptionsResolver.ts | head -100
else
echo "SQS resolver not found"
find . -name "*SqsOptionsResolver*" -type f 2>/dev/null
fiLength of output: 4180
🏁 Script executed:
#!/bin/bash
# Look at all relevant Resolver classes
find packages/app/aws-config -name "*OptionsResolver.ts" | xargs ls -laLength of output: 424
🏁 Script executed:
#!/bin/bash
# Examine the directory structure
tree -L 4 packages/app/aws-config/src/message-queue-toolkit/ 2>/dev/null || find packages/app/aws-config/src/message-queue-toolkit -type f -name "*.ts"Length of output: 480
🏁 Script executed:
#!/bin/bash
# Get lines 180-186 and 206-211 from SNS resolver with more context
python3 << 'EOF'
try:
with open('packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts', 'r') as f:
lines = f.readlines()
print("=== Lines 175-195 ===")
for i in range(174, min(195, len(lines))):
print(f"{i+1}: {lines[i]}", end='')
print("\n=== Lines 201-215 ===")
for i in range(200, min(215, len(lines))):
print(f"{i+1}: {lines[i]}", end='')
except Exception as e:
print(f"Error: {e}")
EOFLength of output: 1192
🏁 Script executed:
#!/bin/bash
# Search in base class for resolveQueueUrl and related methods
python3 << 'EOF'
import os
import re
base_path = 'packages/app/aws-config/src/message-queue-toolkit'
if os.path.exists(base_path):
for file in os.listdir(base_path):
if file.endswith('.ts') and 'test' not in file and 'spec' not in file:
with open(os.path.join(base_path, file), 'r') as f:
content = f.read()
if 'resolveQueueUrl' in content or 'resolveDlq' in content or 'resolveKmsKeyId' in content:
print(f"\n=== {file} ===")
lines = content.split('\n')
for i, line in enumerate(lines):
if 'KmsMasterKeyId' in line or 'resolveKmsKeyId' in line:
start = max(0, i-3)
end = min(len(lines), i+4)
for j in range(start, end):
prefix = ">>> " if j == i else " "
print(f"{prefix}{j+1}: {lines[j]}")
else:
print("Path not found")
EOFLength of output: 2075
🏁 Script executed:
#!/bin/bash
# Comprehensive analysis - get full content of all resolver files
python3 << 'EOF'
import os
base_path = 'packages/app/aws-config/src/message-queue-toolkit'
resolver_files = []
if os.path.exists(base_path):
for file in sorted(os.listdir(base_path)):
if file.endswith('Resolver.ts') and 'test' not in file:
full_path = os.path.join(base_path, file)
resolver_files.append((file, full_path))
for name, path in resolver_files:
print(f"\n{'='*60}")
print(f"FILE: {name}")
print('='*60)
try:
with open(path, 'r') as f:
for i, line in enumerate(f, 1):
print(f"{i:3d}: {line}", end='')
except Exception as e:
print(f"Error reading: {e}")
else:
print("Directory not found")
EOFLength of output: 19771
🏁 Script executed:
#!/bin/bash
# Get all test/spec files in the aws-config area
find packages/app/aws-config -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | head -10Length of output: 654
🏁 Script executed:
#!/bin/bash
# Search for test files that contain KMS or resolver tests
find packages/app -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "resolveKmsKeyId\|alias/aws" {} \; | head -5Length of output: 177
🏁 Script executed:
#!/bin/bash
# If tests exist, examine them
python3 << 'EOF'
import os
import subprocess
# Find test files related to resolvers
result = subprocess.run(
['find', 'packages/app', '-type', 'f', '-name', '*.test.ts', '-o', '-name', '*.spec.ts'],
capture_output=True, text=True
)
test_files = result.stdout.strip().split('\n')
relevant_tests = [f for f in test_files if 'resolver' in f.lower() and f.strip()]
for test_file in relevant_tests[:3]:
if test_file and os.path.exists(test_file):
print(f"\n{'='*60}\nTEST FILE: {test_file}\n{'='*60}")
with open(test_file, 'r') as f:
content = f.read()
lines = content.split('\n')
for i, line in enumerate(lines[:150], 1):
print(f"{i:3d}: {line}")
EOFLength of output: 10966
🏁 Script executed:
#!/bin/bash
# Last resort: show what's actually in the directory
find packages/app/aws-config/src/message-queue-toolkit -maxdepth 1 -type f | sortLength of output: 759
🏁 Script executed:
#!/bin/bash
# Try to locate the file with find
find packages/app -name "MessageQueueToolkitSnsOptionsResolver.ts" -type fLength of output: 90
🏁 Script executed:
#!/bin/bash
# If path is different, search for any *SnSOptions* file
find packages -name "*ns*ptions*" -type f -iname "*.ts"Length of output: 90
🏁 Script executed:
#!/bin/bash
# Display git status if available to see what files exist
ls -la packages/app/aws-config/src/message-queue-toolkit/ 2>/dev/null || echo "Directory does not exist or not accessible"Length of output: 936
🏁 Script executed:
#!/bin/bash
# Escape attempt - show directory listing
find packages/app/aws-config/src -type f -name "*.ts" | sortLength of output: 1629
🏁 Script executed:
#!/bin/bash
# Check if packages/app even exists
[ -d packages/app ] && echo "packages/app exists" || echo "packages/app does not exist"
[ -d packages/app/aws-config ] && echo "aws-config exists" || echo "aws-config does not exist"
find packages/app -type d -name "message-queue-toolkit" 2>/dev/nullLength of output: 87
🏁 Script executed:
#!/bin/bash
# List all TypeScript files to find the actual structure
find packages -name "*.ts" -path "*message*queue*" -o -name "*SnSOptions*" 2>/dev/null | head -20Length of output: 956
🏁 Script executed:
#!/bin/bash
# Very basic - what files exist
ls -R packages/app/aws-config/src 2>/dev/null | head -50Length of output: 827
🏁 Script executed:
#!/bin/bash
# Search for resolveKmsKeyId across entire codebase
rg "resolveKmsKeyId" --type ts -lLength of output: 277
🏁 Script executed:
#!/bin/bash
# Search for any file containing these classes/methods
rg "MessageQueueToolkitSnsOptionsResolver|resolveTopic.*KmsMasterKeyId" --type ts -lLength of output: 238
🏁 Script executed:
#!/bin/bash
# Get file listing differently
find . -path "*/message-queue-toolkit/*.ts" -type f 2>/dev/nullLength of output: 980
🏁 Script executed:
#!/bin/bash
# Broadest possible search
find . -name "*SnsOptions*" -o -name "*resolveKmsKeyId*" 2>/dev/nullLength of output: 92
🏁 Script executed:
#!/bin/bash
# Try Python to read directly
python3 -c "import os; print(os.path.exists('packages/app/aws-config/src/message-queue-toolkit/MessageQueueToolkitSnsOptionsResolver.ts'))"Length of output: 4
🏁 Script executed:
#!/bin/bash
# List entire packages directory structure level by level
echo "=== packages structure ===" && ls -la packages/ && echo "=== app structure ===" && ls -la packages/app 2>/dev/null || echo "packages/app not found"Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Get pwd and list
pwd && find . -maxdepth 3 -type d -name "aws*" -o -name "message*"Length of output: 44
🏁 Script executed:
#!/bin/bash
# Ultimate fallback - search everything
rg "KmsMasterKeyId" --type ts -B 5 -A 5 | head -100Length of output: 11411
🏁 Script executed:
#!/bin/bash
# Search for the exact methods mentioned
rg "resolveTopic|resolveQueueUrl|resolveDlq|resolveKmsKeyId" --type ts | head -30Length of output: 1445
🏁 Script executed:
#!/bin/bash
# Document structure
find . -type f -name "*.ts" | grep -i "option.*resolver\|resolver.*option" | head -10Length of output: 283
🏁 Script executed:
#!/bin/bash
# Check if it's a different path pattern
rg "class.*SnsOptionsResolver" --type ts -B 2 -A 10Length of output: 1714
🏁 Script executed:
#!/bin/bash
# If all else fails, get AWS documentation
echo "Checking AWS documentation for service-managed key conventions..."
# This will be a web search insteadLength of output: 65
🏁 Script executed:
#!/bin/bash
# One more comprehensive attempt with rg on full class definitions
rg "^(class|export|import).*Sns.*Resolver" --type ts -C 20Length of output: 8742
🏁 Script executed:
#!/bin="bash
# Try ast-grep to find class definitions
ast-grep --pattern 'class $_ extends AbstractMessageQueueToolkitOptionsResolver { $$$ }'Length of output: 31340
🏁 Script executed:
#!/bin/bash
# Simple recursive type-script file list
find . -type f -name "*.ts" -not -path "./node_modules/*" | grep -i "resolver" | head -20Length of output: 465
🏁 Script executed:
#!/bin/bash
# Last attempt: direct rg search for the method and class together
rg "class.*Sns.*Resolver.*{|resolveKmsKeyId.*alias/aws/sns" --type ts --multiline-dotall -A 5Length of output: 159
🏁 Script executed:
#!/bin/bash
# Get any file with both Sns and resolveKmsKeyId
rg -l "Sns.*resolveKmsKeyId|resolveKmsKeyId.*Sns" --type tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Try to match partial filename
find . -iname "*sns*resolver*" -type f 2>/dev/nullLength of output: 183
🏁 Script executed:
#!/bin/bash
# Check if directory exists first
test -d packages/app/aws-config/src/message-queue-toolkit && find packages/app/aws-config/src/message-queue-toolkit -type f -name "*.ts" || echo "Directory not found, searching..." && find . -type d -name "message-queue-toolkit"Length of output: 1008
🏁 Script executed:
#!/bin/bash
# Show top-level structure
tree -L 2 -d 2>/dev/null || find . -maxdepth 2 -type d | head -30Length of output: 53
🏁 Script executed:
#!/bin/bash
# Is this maybe a monorepo with different structure
find . -name "*.ts" -path "*resolver*" -path "*queue*" -type f 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if repo is cloned correctly
git log --oneline -5 2>/dev/null || echo "Not a git repo"
git status 2>/dev/null | head -20 || echo "Git not available"Length of output: 308
🌐 Web query:
AWS service-managed KMS keys alias/aws/sns vs alias/aws/sqs documentation
💡 Result:
Short answer
- alias/aws/sns and alias/aws/sqs are AWS-managed KMS aliases (reserved alias/aws/ prefix) that identify AWS-managed KMS keys created and used on behalf of your account and Region by SNS and SQS respectively. You cannot fully manage their lifecycle or key policy; the service creates the key (on first use via console/API/publish) and uses it for SSE by default unless you supply a customer key. [1][2][3]
Key points / differences
- Both are AWS-managed keys with alias names of the form alias/aws/. They’re reserved (you cannot create alias/aws/* yourself). [1]
- SNS: when you enable topic SSE or publish to an SSE-enabled topic, the AWS-managed key for SNS (alias/aws/sns) will be created/used unless you specify a customer (or customer-managed) key. You can also supply a customer-managed CMK and must add SNS permissions to that key’s policy. [2]
- SQS: SQS supports SSE-KMS using alias/aws/sqs (created/used automatically if you don’t specify a CMK). In addition SQS offers SSE-SQS (SQS-managed encryption) as a service-managed option that does not require you to manage KMS keys or charges. If you choose SSE-KMS you may instead supply a customer-managed CMK. [3]
References
- AWS KMS aliases and AWS-managed keys (alias/aws/ prefix, reserved behavior). [1]
- Amazon SNS server-side encryption docs (alias/aws/sns behavior and guidance). [2]
- Amazon SQS encryption docs and SSE‑SQS announcement (alias/aws/sqs behavior; SSE‑SQS service-managed option). [3]
🌐 Web query:
AWS KMS permissions SNS topic SQS queue different service-managed keys
💡 Result:
Short answer: If an SNS topic and its subscribed SQS queue use AWS-managed (service) KMS keys that are different, delivery will often fail. You must use customer-managed CMKs (or at least a CMK you control) and give the SNS service principal explicit KMS permissions on the SQS queue’s CMK (and vice‑versa when required). AWS-managed/service keys’ policies are not editable and don’t grant the cross-service KMS actions SNS needs.
What to do (minimal steps)
- Use a customer-managed KMS key (CMK) for the SQS queue (or for the SNS topic) instead of the AWS-managed key. [1][2]
- Add a key policy statement on that CMK granting the SNS service principal permission to call kms:GenerateDataKey* and kms:Decrypt (and any other required actions). Restrict by SourceArn to the SNS topic ARN when possible. Example (trimmed):
{
"Effect":"Allow",
"Principal":{"Service":"sns.amazonaws.com"},
"Action":["kms:GenerateDataKey*","kms:Decrypt"],
"Resource":"*",
"Condition":{"StringEquals":{"kms:EncryptionContext:aws:sns:topicArn":"arn:aws:sns:...:your-topic"}}
}
This lets SNS generate/decrypt data keys to deliver messages into the encrypted SQS queue. [1][3] - Verify any publisher/service calling SNS also has permission to use the topic’s CMK if the topic is encrypted. [1][3]
References
- SNS docs: setting up topic encryption with encrypted SQS subscriptions. [1]
- SQS key management / KMS guidance. [2]
- AWS knowledge-center / re:Post guide on SNS→SQS and CMK key policy requirements. [3]
If you want, I can produce a ready-to-deploy key policy JSON for your specific topic and queue ARNs and account IDs.
🌐 Web query:
Can SNS topic use alias/aws/sns for SQS queue encryption key ID
💡 Result:
Short answer: Yes for SNS itself, but not for delivering to an encrypted SQS queue.
- An SNS topic may use the AWS-managed key alias/aws/sns as its KMS key. [1]
- However, if an SQS queue is encrypted with the AWS-managed SQS key (alias/aws/sqs) SNS cannot be granted the necessary KMS permissions (you can't modify AWS-managed key policies). To allow SNS to deliver to an encrypted queue you must use a customer‑managed CMK for the SQS queue and add a key policy statement that permits the sns.amazonaws.com principal (and any required aws:SourceArn/SourceAccount conditions). [2]
References:
[1] AWS SNS SSE docs (alias/aws/sns).
[2] AWS guidance for SNS→SQS with encrypted queues / cross-account: create CMK + key policy allowing SNS.
Verify and clarify: SQS queues in SNS resolver must use service-appropriate KMS keys or a customer-managed key
The concern is valid. When MessageQueueToolkitSnsOptionsResolver calls the inherited resolveQueue() method (lines 137–141), that method uses the SNS resolver's resolveKmsKeyId() override, which returns 'alias/aws/sns' for all queue attributes (both regular queues at line 157 and DLQs at line 119 of AbstractMessageQueueToolkitOptionsResolver.ts).
Per AWS documentation, AWS-managed keys (alias/aws/sns and alias/aws/sqs) are service-specific and have non-editable policies. If SNS delivers to an SQS queue encrypted with a different AWS-managed key, the delivery will fail because AWS-managed key policies cannot be modified to grant SNS the required KMS permissions. Tests currently assert queues and DLQs use 'alias/aws/sns' (lines 751, 769, 806 in MessageQueueToolkitSnsResolver.spec.ts), but this creates a cross-service KMS key mismatch.
To fix: either switch SQS queues to use a customer-managed CMK (with appropriate SNS permissions in the key policy), or refactor resolveKmsKeyId() to accept a resource type parameter so topics and queues can use their respective service-managed aliases.
| it('should use default kms key if was not provided explicitly', () => { | ||
| const result = resolver.resolveConsumerOptions(queueName, { | ||
| logger, | ||
| awsConfig: buildAwsConfig(), | ||
| handlers: [], | ||
| }) | ||
|
|
||
| expect(result).toMatchInlineSnapshot(` | ||
| { | ||
| "concurrentConsumersAmount": undefined, | ||
| "consumerOverrides": { | ||
| "batchSize": undefined, | ||
| "heartbeatInterval": 20, | ||
| }, | ||
| "creationConfig": { | ||
| "forceTagUpdate": undefined, | ||
| "policyConfig": { | ||
| "resource": Symbol(current_queue), | ||
| "statements": { | ||
| "Action": [ | ||
| "sqs:SendMessage", | ||
| "sqs:GetQueueAttributes", | ||
| "sqs:GetQueueUrl", | ||
| ], | ||
| "Effect": "Allow", | ||
| "Principal": "*", | ||
| }, | ||
| }, | ||
| "queue": { | ||
| "Attributes": { | ||
| "KmsMasterKeyId": "test kmsKeyId", | ||
| "VisibilityTimeout": "60", | ||
| }, | ||
| "QueueName": "test-mqt-queue_first", | ||
| "tags": { | ||
| "env": "dev", | ||
| "lok-cost-service": "service 1", | ||
| "lok-cost-system": "my-system", | ||
| "lok-owner": "team 1", | ||
| "project": "my-project", | ||
| "service": "sqs", | ||
| }, | ||
| }, | ||
| "updateAttributesIfExists": true, | ||
| }, | ||
| "deadLetterQueue": { | ||
| "creationConfig": { | ||
| "queue": { | ||
| "Attributes": { | ||
| "KmsMasterKeyId": "test kmsKeyId", | ||
| "MessageRetentionPeriod": "604800", | ||
| }, | ||
| "QueueName": "test-mqt-queue_first-dlq", | ||
| "tags": { | ||
| "env": "dev", | ||
| "lok-cost-service": "service 1", | ||
| "lok-cost-system": "my-system", | ||
| "lok-owner": "team 1", | ||
| "project": "my-project", | ||
| "service": "sqs", | ||
| }, | ||
| }, | ||
| "updateAttributesIfExists": true, | ||
| }, | ||
| "redrivePolicy": { | ||
| "maxReceiveCount": 5, | ||
| }, | ||
| }, | ||
| "deletionConfig": { | ||
| "deleteIfExists": undefined, | ||
| }, | ||
| "handlerSpy": undefined, | ||
| "handlers": [], | ||
| "locatorConfig": undefined, | ||
| "logMessages": undefined, | ||
| "maxRetryDuration": 172800, | ||
| "messageTypeField": "type", | ||
| } | ||
| `) | ||
| }) |
There was a problem hiding this comment.
Consumer default-KMS test does not exercise the default path
The test it('should use default kms key if was not provided explicitly', ...) currently calls buildAwsConfig() (which sets kmsKeyId: 'test kmsKeyId'), and the snapshot asserts that value rather than the default alias. So it’s actually verifying “explicit key is respected,” not “default key is used.”
To test the default-key behavior (consistent with the publisher tests), you likely want:
- const result = resolver.resolveConsumerOptions(queueName, {
- logger,
- awsConfig: buildAwsConfig(),
- handlers: [],
- })
+ const result = resolver.resolveConsumerOptions(queueName, {
+ logger,
+ awsConfig: buildAwsConfig({ kmsKeyId: '' }),
+ handlers: [],
+ })…and then update the inline snapshot to expect KmsMasterKeyId: "alias/aws/sqs" for both the main queue and the DLQ. Alternatively, if the intent is to test explicit-key usage, renaming the test accordingly would remove the confusion.
Committable suggestion skipped: line range outside the PR's diff.
| /** Use default KmsKeyId if it's not explicitly provided: | ||
| * - aws/sns for SNS config | ||
| * - aws/sqs for SQS config | ||
| * */ | ||
| useDefaultKmsKeyId?: boolean |
There was a problem hiding this comment.
Clarify docstring to match actual default KMS aliases
The comment says the defaults are aws/sns and aws/sqs, but the resolvers return 'alias/aws/sns' and 'alias/aws/sqs'. To avoid confusion, consider updating the JSDoc to include the alias/ prefix:
- /** Use default KmsKeyId if it's not explicitly provided:
- * - aws/sns for SNS config
- * - aws/sqs for SQS config
- * */
+ /** Use default KmsKeyId if it's not explicitly provided:
+ * - alias/aws/sns for SNS config
+ * - alias/aws/sqs for SQS config
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Use default KmsKeyId if it's not explicitly provided: | |
| * - aws/sns for SNS config | |
| * - aws/sqs for SQS config | |
| * */ | |
| useDefaultKmsKeyId?: boolean | |
| /** Use default KmsKeyId if it's not explicitly provided: | |
| * - alias/aws/sns for SNS config | |
| * - alias/aws/sqs for SQS config | |
| */ | |
| useDefaultKmsKeyId?: boolean |
🤖 Prompt for AI Agents
In packages/app/aws-config/src/message-queue-toolkit/types.ts around lines 22 to
26, update the JSDoc for useDefaultKmsKeyId so it accurately reflects the actual
default KMS key aliases returned by the resolvers by changing the documented
defaults from "aws/sns" and "aws/sqs" to include the "alias/" prefix (i.e.,
"alias/aws/sns" and "alias/aws/sqs"), ensuring the comment matches runtime
behavior and avoids confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/app/aws-config/README.md (1)
243-246: Documentation accurately captures new constructor parameters — minor capitalization fix.The new "Constructor params" section clearly documents
validateNamePatternsanduseDefaultKmsKeyId, with appropriate references to regex constants and specific default aliases for SNS/SQS. This aligns well with the PR objectives.One small fix: Line 246 should use
KMSinstead ofKmsfor consistency with AWS terminology and the rest of the document.~- `useDefaultKmsKeyId` - if set to true, default Kms Key Id will be used if not provided explicitly (`alias/aws/sns` and `alias/aws/sqs` respectively) +- `useDefaultKmsKeyId` - if set to true, default KMS Key Id will be used if not provided explicitly (`alias/aws/sns` and `alias/aws/sqs` respectively)
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/app/aws-config/README.md(1 hunks)
| #### Constructor params | ||
|
|
||
| - `validateNamePatterns` - if set to true, topic/queue names will be validated against regex (see `TOPIC_NAME_REGEX` and `QUEUE_NAME_REGEX`) | ||
| - `useDefaultKmsKeyId` - if set to true, default Kms Key Id will be used if not provided explicitly (`alias/aws/sns` and `alias/aws/sqs` respectively) |
There was a problem hiding this comment.
Could we document default values? what happens if there are not specifyed
| return awsConfig.kmsKeyId | ||
| } | ||
|
|
||
| return 'alias/aws/sns' |
There was a problem hiding this comment.
Could we add those as constants?
Changes
translation-memory-importtopic which is matches our guidelines but fails on validationChecklist
major,minor,patchorskip-releaseSummary by CodeRabbit
New Features
Improvements
Tests
Documentation