-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
AWS Lambda, add optional credential settings to config. #34121
AWS Lambda, add optional credential settings to config. #34121
Conversation
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
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.
Thanks for adding this!
|
||
// Specifies the credentials to be used. This parameter is optional. | ||
// If set, it will override other providers and takes precedence, the provider chain is limited | ||
// to the configuration credentials provider. Other providers are ignored |
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.
Please clarify what other providers, and what's the precedence if credentials_profile
is supplied as well?
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.
when I mention providers is the providers chain following this order https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/aws_lambda_filter#credentials
and explicit credentials should have preference over credentials_profile. I have added test for that but I agree it is not clear, I will try to explain better here.
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
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.
Thanks.
Assigning @suniltheta as code-owner.
/assign @suniltheta
@@ -62,6 +62,24 @@ message Config { | |||
// the provider chain is limited to the AWS credentials file Provider. | |||
// Other providers are ignored | |||
string credentials_profile = 5; | |||
|
|||
// Specifies the credentials to be used. This parameter is optional and if it is set, | |||
// it will override other providers and will take precedence over credentials_profile. |
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.
Please add a similar comment (but inverted) to the credentials_profile
field.
// Specifies the credentials to be used. This parameter is optional and if it is set, | ||
// it will override other providers and will take precedence over credentials_profile. | ||
// The provider chain is limited to the configuration credentials provider. | ||
// Default providers chain specified in documentation will be ignored. |
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.
Not sure I fully understand. Do you mean to say:
If this field is provided, then the default providers chain specified in documentation will be ignored.
?
Can you please elaborate what documentation is referred here? Consider adding a link.
/assign @suniltheta |
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
/wait |
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
// it will override other providers and will take precedence over credentials_profile. | ||
// The provider chain is limited to the configuration credentials provider. | ||
// If this field is provided, then the default providers chain specified in documentation will be ignored. | ||
// (See :ref:`default credentials providers <config_http_filters_aws_lambda_credentials>`) |
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.
can you add a statement that this is not AWS recommended way to providing the credentials? Typical usecase would be to use in testing environment.
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.
sure. would you like me to add an specific text for this? I know Roles is the recommendation in AWS deployments, but I prefer to add the correct note.
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.
something like "Warning! Distributing the AWS credentials via this configuration should not be done in production." should work
string secret_access_key = 2 [(validate.rules).string = {min_len: 1}]; | ||
|
||
// AWS session token | ||
string session_token = 3 [(validate.rules).string = {min_len: 1}]; |
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.
this field could be made optional I think
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.
apparently the legacy environment provider (ENV VARs) it is optional, but if not set the call fails with 403, should I made it optional anyway?
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.
My fault, AWS Docs explains when it is required (the credentials I'm using are temporal)
AWS_SESSION_TOKEN
Specifies the session token value that is required if you are using temporary security credentials that you retrieved directly from AWS STS operations. For more information, see the [Output section of the assume-role command](https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role.html#output) in the AWS CLI Command Reference.
If defined, this environment variable overrides the value for the profile setting aws_session_token.
changelogs/current.yaml
Outdated
change: | | ||
The ``aws_lambda`` filter now supports the | ||
:ref:`credentials <envoy_v3_api_field_extensions.filters.http.aws_lambda.v3.Config.credentials>` parameter. | ||
This enables setting credential from the filter configuration. |
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.
setting AWS credentials from the filter configuration
..
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.
Thanks!
Left a few minor comments.
/assign-from @envoyproxy/senior-maintainers
// | ||
// .. warning:: | ||
// Distributing the AWS credentials via this configuration should not be done in production. | ||
Credentials credentials = 6 [(validate.rules).message = {required: false}]; |
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.
nit: remove [(validate.rules).message = {required: false}]
as this is the default message fields.
@@ -62,6 +62,11 @@ constexpr char STS_TOKEN_CLUSTER[] = "sts_token_service_internal"; | |||
|
|||
} // namespace | |||
|
|||
Credentials ConfigCredentialsProvider::getCredentials() { | |||
ENVOY_LOG(debug, "Getting AWS credentials from configuration"); |
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.
nit:
ENVOY_LOG(debug, "Getting AWS credentials from configuration"); | |
ENVOY_LOG(debug, "Getting AWS credentials from static configuration"); |
@@ -45,6 +45,19 @@ class EnvironmentCredentialsProvider : public CredentialsProvider, | |||
Credentials getCredentials() override; | |||
}; | |||
|
|||
class ConfigCredentialsProvider : public CredentialsProvider, |
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.
Please add a class comment.
absl::string_view secret_access_key = absl::string_view(), | ||
absl::string_view session_token = absl::string_view()) | ||
: credentials_(access_key_id, secret_access_key, session_token) {} | ||
Credentials getCredentials() override; |
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.
OOC why is this returned by value, and not const ref?
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.
unfortunatelly it is part of the interfase https://github.com/envoyproxy/envoy/blob/main/source/extensions/common/aws/credentials_provider.h#L68
on the other providers it is temporal, or cached. cannot make the method const neither.
I will change credentials_ to const for correct constness
// default providers chain, it will use the credentials file provider with | ||
// the configured profile. All other providers will be ignored. | ||
// In case credentials from config or credentials_profile are set in the configuration, instead of | ||
// using the default providers chain, it will use the credentials from config as first option then |
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.
nit:
// using the default providers chain, it will use the credentials from config as first option then | |
// using the default providers chain, it will use the credentials from config (if provided), then |
Credentials credentials = 6 [(validate.rules).message = {required: false}]; | ||
} | ||
|
||
// AWS Lambda Credentials config |
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.
style-nit: Please add '.' at end of comments (here and in other places).
string secret_access_key = 2 [(validate.rules).string = {min_len: 1}]; | ||
|
||
// AWS session token | ||
string session_token = 3; |
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.
Is the default value an empty string. Can you add a small comment describing what happens when it is not set?
provider)); | ||
} | ||
|
||
TEST(AwsLambdaFilterConfigTest, GetProviderShoudPrioritizeProfileIfNoCredentials) { |
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.
TEST(AwsLambdaFilterConfigTest, GetProviderShoudPrioritizeProfileIfNoCredentials) { | |
TEST(AwsLambdaFilterConfigTest, GetProviderShouldPrioritizeProfileIfNoCredentials) { |
@envoyproxy/senior-maintainers assignee is @alyssawilk |
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
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.
Small nits, but otherwise lgtm.
@@ -60,8 +60,34 @@ message Config { | |||
// Specifies the credentials profile to be used from the AWS credentials file. | |||
// This parameter is optional. If set, it will override the value set in the AWS_PROFILE env variable and | |||
// the provider chain is limited to the AWS credentials file Provider. | |||
// Other providers are ignored | |||
// If credentials configuration is provided, this configuration will be ignored. | |||
// If this field is provided, then the default providers chain specified in documentation will be ignored. |
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.
// If this field is provided, then the default providers chain specified in documentation will be ignored. | |
// If this field is provided, then the default providers chain specified in the documentation will be ignored. |
// Specifies the credentials to be used. This parameter is optional and if it is set, | ||
// it will override other providers and will take precedence over credentials_profile. | ||
// The provider chain is limited to the configuration credentials provider. | ||
// If this field is provided, then the default providers chain specified in documentation will be ignored. |
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.
// If this field is provided, then the default providers chain specified in documentation will be ignored. | |
// If this field is provided, then the default providers chain specified in the documentation will be ignored. |
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
@adisuissa generally prefered to throw extension reviews to extension owner rather than assign-from senior maintainers. Passing over to Matt. |
Commit Message: aws_lambda: add optional credential settings to config.
Additional Description: Adds credentials setting to configuration. This new configuration is optional, but if it is set, it will take precedence over the optional profile configuration and legacy provider chain.
Risk Level: Low
Testing: yes
Docs Changes: yes
Release Notes: yes
Platform Specific Features: n/a