chore: clean up authorization/environment logic #570
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Updated the default value of |
|
@rustatian Can you try running the tests now? I set |
rustatian
changed the title
Deduplicate checkEnv code Deduplicate GetQueueUrl code Default to empty endpoint Update dependencies, use WithBaseEndpoint Remove "no global section" test Add log statement for AWS IAM config assumption
WalkthroughThe changes in this pull request involve updates to dependency versions in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SQSDriver
participant AWSConfig
User->>SQSDriver: Initialize SQS
SQSDriver->>AWSConfig: Load AWS Configuration
AWSConfig-->>SQSDriver: Return Configuration
SQSDriver->>User: SQS Initialized
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
- go.mod (1 hunks)
- sqsjobs/config.go (0 hunks)
- sqsjobs/driver.go (7 hunks)
- tests/configs/.rr-no-global.yaml (0 hunks)
- tests/go.mod (4 hunks)
- tests/jobs_sqs_test.go (1 hunks)
💤 Files with no reviewable changes (2)
- sqsjobs/config.go
- tests/configs/.rr-no-global.yaml
🧰 Additional context used
🔇 Additional comments (14)
go.mod (1)
9-9: Approve the AWS SDK config package update.The update of
github.com/aws/aws-sdk-go-v2/configfrom v1.27.43 to v1.28.0 is appropriate and likely supports the changes in authorization and environment logic mentioned in the PR objectives.To ensure compatibility, please verify that this version update supports the removal of EC2 detection and changes in configuration handling. Run the following script to check for any breaking changes or new features in the changelog:
tests/go.mod (7)
26-26: LGTM: RoadRunner SQS update looks good.The update of
github.com/roadrunner-server/sqs/v5from v5.0.0 to v5.0.4 is a patch version update, which typically includes bug fixes and minor improvements. This update is likely beneficial for the project.To understand the changes included in this update, please check the changelog:
#!/bin/bash echo "Fetching RoadRunner SQS v5.0.4 changelog..." curl -s https://api.github.com/repos/roadrunner-server/sqs/releases/tags/v5.0.4 | jq -r '.body'
119-119: Verify stability of genproto/googleapis/api update.The update of
google.golang.org/genproto/googleapis/apito v0.0.0-20241015192408-796eee8c2d53 points to a specific commit. While this update is likely necessary for compatibility with other Google APIs, it's important to ensure it's stable for your use case.To check the changes introduced in this commit, run:
#!/bin/bash echo "Fetching changes for genproto/googleapis/api commit 796eee8c2d53..." git ls-remote https://github.com/googleapis/go-genproto.git | grep 796eee8c2d53
71-71: LGTM: Prometheus client update is appropriate.The update of
github.com/prometheus/client_golangfrom v1.20.4 to v1.20.5 is a patch version update, which typically includes bug fixes. This update is likely to improve the stability of the project.To understand the specific changes in this update, please check the changelog:
#!/bin/bash echo "Fetching Prometheus Client Golang v1.20.5 changelog..." curl -s https://api.github.com/repos/prometheus/client_golang/releases/tags/v1.20.5 | jq -r '.body'
Line range hint
1-126: Overall, the dependency updates look good and should improve the project.The changes in this
go.modfile are focused on updating dependencies to newer versions. Here's a summary of the updates:
- Minor version updates: Toxiproxy and AWS SDK config
- Patch version updates: RoadRunner SQS and Prometheus client
- Specific commit updates: Google genproto packages
These updates are likely to bring bug fixes, performance improvements, and new features. The consistency in updating related packages is good for maintaining compatibility.
However, it's important to:
- Review the changelogs for each updated package to understand the specific changes.
- Run the test suite to ensure these updates don't introduce any regressions.
- Pay special attention to the genproto packages, as they're updated to a specific commit rather than a released version.
To ensure all dependencies are properly synchronized and there are no conflicts, run:
#!/bin/bash echo "Verifying and tidying go.mod..." go mod verify go mod tidy
120-120: Verify stability of genproto/googleapis/rpc update.The update of
google.golang.org/genproto/googleapis/rpcto v0.0.0-20241015192408-796eee8c2d53 points to the same commit as the previous genproto package. This consistency is good, but it's important to ensure it's stable for your use case.To check the changes introduced in this commit, run:
#!/bin/bash echo "Fetching changes for genproto/googleapis/rpc commit 796eee8c2d53..." git ls-remote https://github.com/googleapis/go-genproto.git | grep 796eee8c2d53
10-10: LGTM: AWS SDK config update is consistent.The update of
github.com/aws/aws-sdk-go-v2/configfrom v1.27.43 to v1.28.0 aligns with the update made in the main module's go.mod file. This consistency is good for maintaining compatibility between the main code and tests.To ensure there are no breaking changes, please check the changelog for this version:
#!/bin/bash echo "Fetching AWS SDK Go v2 Config v1.28.0 changelog..." curl -s https://api.github.com/repos/aws/aws-sdk-go-v2/releases/tags/config/v1.28.0 | jq -r '.body'
8-8: LGTM: Toxiproxy update looks good.The update of
github.com/Shopify/toxiproxy/v2from v2.10.0 to v2.11.0 is a minor version bump, which typically includes new features and bug fixes. This update is likely beneficial for the project.To ensure there are no breaking changes, please check the changelog for this version:
✅ Verification successful
LGTM: Toxiproxy update verified.
The update of
github.com/Shopify/toxiproxy/v2from v2.10.0 to v2.11.0 involves unpinning the minimum patch version. As per PR #595, this change was made because the library is only used for testing, and unpinning allows the Go toolchain to select the appropriate version, requiring at least version1.22. This change should not adversely affect the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Fetching Toxiproxy v2.11.0 changelog..." curl -s https://api.github.com/repos/Shopify/toxiproxy/releases/tags/v2.11.0 | jq -r '.body'Length of output: 423
Script:
#!/bin/bash # Fetching Toxiproxy v2.11.0 changelog curl -s https://api.github.com/repos/Shopify/toxiproxy/releases/tags/v2.11.0 | jq -r '.body'Length of output: 157
Script:
#!/bin/bash # Fetching details of PR #595 from Toxiproxy repository curl -s https://api.github.com/repos/Shopify/toxiproxy/pulls/595 | jq '.title, .body'Length of output: 630
tests/jobs_sqs_test.go (1)
780-780: LGTM! This change improves configuration flexibility.The modification to use
sqsConf.WithBaseEndpointfor setting the SQS endpoint is a good improvement. It aligns with the PR objectives by making the configuration more explicit and flexible. This approach allows for easier testing and configuration management, especially when working with different environments or custom SQS endpoints.sqsjobs/driver.go (5)
29-31: Verify the Export Status of ConstantsThe constants
pluginName,tracerName, andassumeAWSEnvare declared in lowercase, making them unexported. If these constants are intended for use outside this package, consider capitalizing their names to export them.
135-135: Function Call Updated AppropriatelyThe call to
checkEnvhas been updated to match the new function signature without theinsideAWSparameter. This change aligns with the updated function definition.
178-179: Clarify Log Message Regarding AWS Environment AssumptionThis is a duplicate of a previous comment.
218-218: Function Call Updated AppropriatelyThe
checkEnvfunction is called with the updated signature, which no longer includes theinsideAWSparameter. The modification is correct.
448-486:⚠️ Potential issueEnsure Consistency with Configuration Requirements in
checkEnvFunctionThe
checkEnvfunction defaults to using AWS IAM roles when no credentials are provided:if secret != "" && key != "" { opts = append(opts, config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(key, secret, sessionToken))) }According to the PR objectives, users are required to provide valid configuration parameters explicitly, even when operating on AWS. This default behavior may conflict with the intended changes.
Please verify if defaulting to AWS IAM roles is acceptable within the new configuration requirements. If explicit credentials are mandatory, consider adding validation to enforce this:
if secret == "" || key == "" { return nil, errors.E(op, errors.New("AWS access key and secret are required")) }
|
Thank you, @nickdnk 👍 |
Reason for This PR
As discussed on Discord, the current implementation of the authorization and configuration parameters is kind of inconsistent and relies on manual detection of EC2 environments. This leads to weird combinations of viable configuration parameters that behave differently depending on where the application runs.
Description of Changes
I've removed the detection of EC2 and made the
checkEnvfunction behave the same whether or not it's running on AWS. It is the user's responsibility to provide valid configuration parameters.It is now possible to provide access key and secret without a session token. Session tokens are temporary credentials that expire after at most 12 hours, so hardcoding them in a configuration file doesn't make any sense. We now only check for access key and secret and just override the session token if it's provided, and leave it blank if not.
This is a breaking change as the lack of the EC2 check means you have to provide the
sqsroot config, even if you're running on AWS. You can, however, still providesqswith no keys. I'm sure there's a way around this by moving some code blocks around and checking for populated variables after having tried to load them from the AWS environment.I never coded a day in my life in Go, so there might be some obvious mistakes here. I didn't test, compile or run this code at all, it's just a rough draft.
I don't know how to sign my commit. I see it listed as a requirement.
ref: roadrunner-server/docs#27
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores