AWS Lambda, add optional credential settings to config. #34121

juanmolle · 2024-05-13T21:07:32Z

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

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

repokitteh-read-only · 2024-05-13T21:07:41Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34121 was opened by juanmolle.

see: more, trace.

adisuissa

Thanks for adding this!

adisuissa · 2024-05-13T21:29:18Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+
+  // 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


Please clarify what other providers, and what's the precedence if credentials_profile is supplied as well?

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>

adisuissa

Thanks.
Assigning @suniltheta as code-owner.
/assign @suniltheta

adisuissa · 2024-05-14T15:16:02Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

@@ -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.


Please add a similar comment (but inverted) to the credentials_profile field.

adisuissa · 2024-05-14T15:17:27Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  // 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.


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.

juanmolle · 2024-05-15T12:14:04Z

/assign @suniltheta

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

adisuissa · 2024-05-15T13:38:11Z

/wait

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

suniltheta · 2024-05-15T16:24:44Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  // 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>`)


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.

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.

something like "Warning! Distributing the AWS credentials via this configuration should not be done in production." should work

suniltheta · 2024-05-15T16:25:08Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  string secret_access_key = 2 [(validate.rules).string = {min_len: 1}];
+
+  // AWS session token
+  string session_token = 3 [(validate.rules).string = {min_len: 1}];


this field could be made optional I think

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?

https://github.com/envoyproxy/envoy/blob/main/source/extensions/common/aws/credentials_provider_impl.cc#L74

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.

suniltheta · 2024-05-15T16:26:32Z

changelogs/current.yaml

+  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.


setting AWS credentials from the filter configuration..

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

adisuissa

Thanks!
Left a few minor comments.
/assign-from @envoyproxy/senior-maintainers

adisuissa · 2024-05-16T17:05:30Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  //
+  // .. warning::
+  //   Distributing the AWS credentials via this configuration should not be done in production.
+  Credentials credentials = 6 [(validate.rules).message = {required: false}];


nit: remove [(validate.rules).message = {required: false}] as this is the default message fields.

adisuissa · 2024-05-16T17:06:24Z

source/extensions/common/aws/credentials_provider_impl.cc

@@ -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");


nit:

Suggested change

ENVOY_LOG(debug, "Getting AWS credentials from configuration");

ENVOY_LOG(debug, "Getting AWS credentials from static configuration");

adisuissa · 2024-05-16T17:06:52Z

source/extensions/common/aws/credentials_provider_impl.h

@@ -45,6 +45,19 @@ class EnvironmentCredentialsProvider : public CredentialsProvider,
  Credentials getCredentials() override;
 };

+class ConfigCredentialsProvider : public CredentialsProvider,


Please add a class comment.

adisuissa · 2024-05-16T17:07:33Z

source/extensions/common/aws/credentials_provider_impl.h

+                            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;


OOC why is this returned by value, and not const ref?

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

adisuissa · 2024-05-16T17:09:34Z

source/extensions/filters/http/aws_lambda/config.cc

-// 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


nit:

Suggested change

// 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

adisuissa · 2024-05-16T17:11:39Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  Credentials credentials = 6 [(validate.rules).message = {required: false}];
+}
+
+// AWS Lambda Credentials config


style-nit: Please add '.' at end of comments (here and in other places).

adisuissa · 2024-05-16T17:12:31Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  string secret_access_key = 2 [(validate.rules).string = {min_len: 1}];
+
+  // AWS session token
+  string session_token = 3;


Is the default value an empty string. Can you add a small comment describing what happens when it is not set?

adisuissa · 2024-05-16T17:14:20Z

test/extensions/filters/http/aws_lambda/config_test.cc

+      provider));
+}
+
+TEST(AwsLambdaFilterConfigTest, GetProviderShoudPrioritizeProfileIfNoCredentials) {


Suggested change

TEST(AwsLambdaFilterConfigTest, GetProviderShoudPrioritizeProfileIfNoCredentials) {

TEST(AwsLambdaFilterConfigTest, GetProviderShouldPrioritizeProfileIfNoCredentials) {

repokitteh-read-only · 2024-05-16T17:25:40Z

@envoyproxy/senior-maintainers assignee is @alyssawilk

🐱

Caused by: a #34121 (review) was submitted by @adisuissa.

see: more, trace.

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

adisuissa

Small nits, but otherwise lgtm.

adisuissa · 2024-05-17T17:41:20Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

@@ -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.


Suggested change

// 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.

adisuissa · 2024-05-17T17:41:37Z

api/envoy/extensions/filters/http/aws_lambda/v3/aws_lambda.proto

+  // 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.


Suggested change

// 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>

alyssawilk · 2024-05-20T14:08:38Z

@adisuissa generally prefered to throw extension reviews to extension owner rather than assign-from senior maintainers. Passing over to Matt.

Initial Commit

80544d1

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

juanmolle requested a review from mattklein123 as a code owner May 13, 2024 21:07

repokitteh-read-only bot added the api label May 13, 2024

repokitteh-read-only bot assigned adisuissa May 13, 2024

adisuissa reviewed May 13, 2024

View reviewed changes

rewrite proto doc

7101e9f

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

adisuissa reviewed May 14, 2024

View reviewed changes

repokitteh-read-only bot assigned suniltheta May 15, 2024

Merge branch 'main' into add_aws_lambda_cred_to_config

51fbdd7

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

repokitteh-read-only bot added the waiting label May 15, 2024

clarify docs

e429a93

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

repokitteh-read-only bot removed the waiting label May 15, 2024

Merge branch 'main' into add_aws_lambda_cred_to_config

1e5d5a7

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

suniltheta reviewed May 15, 2024

View reviewed changes

rework

39bb726

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

juanmolle requested a review from suniltheta May 16, 2024 15:09

suniltheta approved these changes May 16, 2024

View reviewed changes

adisuissa reviewed May 16, 2024

View reviewed changes

repokitteh-read-only bot assigned alyssawilk May 16, 2024

feedback rework

8cbe43b

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

adisuissa reviewed May 17, 2024

View reviewed changes

small nits

9f6a8bf

Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>

juanmolle requested a review from adisuissa May 20, 2024 12:10

alyssawilk assigned mattklein123 and unassigned alyssawilk May 20, 2024

mattklein123 approved these changes May 20, 2024

View reviewed changes

repokitteh-read-only bot removed the api label May 20, 2024

mattklein123 merged commit 37df2cd into envoyproxy:main May 20, 2024
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Lambda, add optional credential settings to config. #34121

AWS Lambda, add optional credential settings to config. #34121

juanmolle commented May 13, 2024

repokitteh-read-only bot commented May 13, 2024

adisuissa left a comment

adisuissa May 13, 2024

juanmolle May 14, 2024 •

edited

adisuissa left a comment

adisuissa May 14, 2024

adisuissa May 14, 2024

juanmolle commented May 15, 2024

adisuissa commented May 15, 2024

suniltheta May 15, 2024

juanmolle May 15, 2024

suniltheta May 15, 2024 •

edited

suniltheta May 15, 2024

juanmolle May 15, 2024

juanmolle May 15, 2024

suniltheta May 15, 2024

adisuissa left a comment

adisuissa May 16, 2024

adisuissa May 16, 2024

adisuissa May 16, 2024

adisuissa May 16, 2024

juanmolle May 16, 2024

adisuissa May 16, 2024

adisuissa May 16, 2024

adisuissa May 16, 2024

adisuissa May 16, 2024

repokitteh-read-only bot commented May 16, 2024

adisuissa left a comment

adisuissa May 17, 2024

adisuissa May 17, 2024

alyssawilk commented May 20, 2024

	ENVOY_LOG(debug, "Getting AWS credentials from configuration");
	ENVOY_LOG(debug, "Getting AWS credentials from static configuration");

	// 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

	TEST(AwsLambdaFilterConfigTest, GetProviderShoudPrioritizeProfileIfNoCredentials) {
	TEST(AwsLambdaFilterConfigTest, GetProviderShouldPrioritizeProfileIfNoCredentials) {

	// 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.

AWS Lambda, add optional credential settings to config. #34121

AWS Lambda, add optional credential settings to config. #34121

Conversation

juanmolle commented May 13, 2024

repokitteh-read-only bot commented May 13, 2024

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanmolle May 14, 2024 • edited

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanmolle commented May 15, 2024

adisuissa commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suniltheta May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented May 16, 2024

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented May 20, 2024

juanmolle May 14, 2024 •

edited

suniltheta May 15, 2024 •

edited