aws: rds refresh assertion failure #37752
Draft
+562
−238
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
|
Assigning @suniltheta for a first pass review. |
suniltheta
approved these changes
Dec 20, 2024
suniltheta
reviewed
Dec 20, 2024
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
nbaws
commented
Jan 1, 2025
Signed-off-by: Nigel Brittain <[email protected]>
nbaws
commented
Jan 1, 2025
source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.h
Show resolved
Hide resolved
nbaws
commented
Jan 1, 2025
source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
Signed-off-by: Nigel Brittain <[email protected]>
atanzu
reviewed
Jan 8, 2025
| } | ||
|
|
||
| if (region.empty()) { | ||
| auto region_provider = |
There was a problem hiding this comment.
Why do we need a shared_ptr here? From what I see we neither return region_provider nor store it.
| "AWS region is not set in xDS configuration and failed to retrieve from " | ||
| "environment variable or AWS profile/config files."); | ||
| } | ||
| region = regionOpt.value(); |
There was a problem hiding this comment.
Here we assign a value to a local variable, and then return from the function, is that intended?
| FilterConfigImpl( | ||
| Extensions::Common::Aws::SignerPtr&& signer, | ||
| Envoy::Extensions::Common::Aws::CredentialsProviderSharedPtr credentials_provider, | ||
| const std::string& stats_prefix, Stats::Scope& scope, const std::string& host_rewrite, |
There was a problem hiding this comment.
nit: I'd recommend to use std::string_view instead of const std::string& for string parameters. This is also applicable to other methods which take strings as arguments.
| // Get the Metadata credentials cache duration. | ||
| static std::chrono::seconds getCacheDuration(); | ||
|
|
||
| private: | ||
| void createCluster(bool new_timer); | ||
| void createCluster(bool new_timer, std::string cluster_name, const envoy::config::cluster::v3::Cluster::DiscoveryType cluster_type, std::string uri ); |
| @@ -81,13 +79,14 @@ class SignerBaseImpl : public Signer, public Logger::Loggable<Logger::Id::aws> { | |||
| } | |||
| } | |||
|
|
|||
| absl::Status sign(Http::RequestMessage& message, bool sign_body = false, | |||
| absl::Status sign(Http::RequestMessage& message, const Credentials credentials, | |||
There was a problem hiding this comment.
Here we use a mix of absl::string_view and std::string, would that be possible to use similar types (preferably views)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit Message: Fixes assertion failure during rds refresh with route level extension
Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] Fixes customer reported bug
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]