-
Notifications
You must be signed in to change notification settings - Fork 92
fix(awssecrets): support ARN-style secret identifiers in URIs (#909) #910
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR fixes issue #909 by enabling support for AWS ARN-style secret identifiers in vals URIs. The problem stems from Go's url.Parse misinterpreting colons in AWS ARNs (e.g., arn:aws:secretsmanager:us-east-1:123456789012:secret:/path) as port separators, causing parsing errors. The solution temporarily transforms ARN-based URIs into a triple-slash format that url.Parse can handle without port-related issues, then restores the ARN to the appropriate uri.Host field after parsing.
Key changes:
- Added ARN detection and transformation logic in vals.go to work around url.Parse limitations with colon-heavy identifiers
- Implemented comprehensive test coverage including mock provider tests and multiple ARN format scenarios
- Supports all AWS partitions (aws, aws-cn, aws-us-gov) through flexible pattern matching
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| vals.go | Implements ARN detection, temporary transformation for parsing, and restoration logic to handle AWS ARN identifiers in URI host positions |
| vals_test.go | Adds mock provider implementation and comprehensive test cases covering ARN URIs with/without query parameters and fragments |
| // Find the actual path after the ARN | ||
| arnLen := len(arnValue) | ||
| if len(uri.Path) > arnLen+1 { | ||
| uri.Path = uri.Path[arnLen+1:] | ||
| } else { | ||
| uri.Path = "" | ||
| } |
Copilot
AI
Dec 12, 2025
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.
The comment "Find the actual path after the ARN" on line 380 is potentially misleading. In the current implementation and test cases, the entire ARN (including any path-like components such as /myteam/mydoc) is treated as the resource identifier and placed in uri.Host. The logic here ensures uri.Path is empty when the entire remainder was the ARN, which is the correct behavior. Consider updating this comment to clarify that this handles the edge case where there might be additional path components after the ARN, though in practice for AWS ARNs, the entire identifier including any slashes is part of the ARN itself and should go to the host.
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.
@copilot open a new pull request to apply changes based on this feedback
| "crypto/md5" | ||
| "fmt" |
Copilot
AI
Dec 12, 2025
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.
Import statements should be organized with standard library imports grouped together before third-party imports. The crypto/md5 and fmt imports should be moved up to join the other standard library imports (bytes, os, path/filepath, sort, strings, testing) rather than being placed after testing.
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.
@copilot open a new pull request to apply changes based on this feedback
| // compute provider hash used by Runtime (scheme + query.Encode()) | ||
| hash := fmt.Sprintf("%x", md5.Sum([]byte("echo"))) |
Copilot
AI
Dec 12, 2025
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.
The hash calculation on line 218 only includes the scheme ("echo") but not the query parameters. According to the uriToProviderHash function in vals.go (lines 163-169), the hash should include both uri.Scheme and uri.Query().Encode(). Since this test uses "ref+echo://..." with no query parameters, the query would be empty, so the current calculation happens to be correct. However, the comment is misleading - it says "scheme + query.Encode()" but the code only uses the scheme. Consider updating the comment to clarify that this works because there are no query parameters, or better yet, use the actual uriToProviderHash logic to compute the hash to make this test more robust and maintainable.
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.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes #909
Problem
Go's treats colons in AWS ARNs as port separators, causing parsing errors for full ARN URIs such as:
Solution
Tests
Files changed
Notes