feat(parameters): accept boto3_client to support private endpoints and ease testing #1096

ran-isenberg · 2022-04-06T17:27:10Z

Fixes: #1079

Summary

Changes

Please provide a summary of what's being changed

This adds a new boto3_client constructor parameter to the Parameters feature to unblock customers using VPC endpoints.

~~I didn't add it to DynamoDB parameter because i'm not sure it's possible there. Fixed also the docs.~~ All clients have it now, and DynamoDB has a note in docstrings + type referring to Resource Client (high-level) not Client (low-level) like SSM, SecretsManager and AppConfig.

User experience

Please share what the user experience looks like before and after this change

import boto3

from aws_lambda_powertools.utilities import parameters

# Initialize clients with custom VPC endpoint
ssm = boto3.client("ssm", endpoint_url="custom_endpoint")
secrets = boto3.client("secretsmanager", endpoint_url="custom_endpoint")
appconfig = boto3.client("appconfig", endpoint_url="custom_endpoint")


ssm_provider = parameters.SSMProvider(boto3_client=ssm)
secrets_provider = parameters.SecretsProvider(boto3_client=secrets)
appconf_provider = parameters.AppConfigProvider(environment="my_env", application="my_app", boto3_client=appconfig)

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Meet tenets criteria
I have performed a self-review of my this change
Changes are tested
Changes are documented
PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

Migration process documented
Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

View rendered docs/utilities/parameters.md

codecov-commenter · 2022-04-06T17:38:33Z

Codecov Report

Merging #1096 (cec2f33) into develop (0777858) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1096   +/-   ##
========================================
  Coverage    99.88%   99.88%           
========================================
  Files          119      119           
  Lines         5414     5425   +11     
  Branches       616      618    +2     
========================================
+ Hits          5408     5419   +11     
  Misses           2        2           
  Partials         4        4

Impacted Files	Coverage Δ
...ambda_powertools/utilities/parameters/appconfig.py	`100.00% <100.00%> (ø)`
aws_lambda_powertools/utilities/parameters/base.py	`100.00% <100.00%> (ø)`
...lambda_powertools/utilities/parameters/dynamodb.py	`100.00% <100.00%> (ø)`
..._lambda_powertools/utilities/parameters/secrets.py	`100.00% <100.00%> (ø)`
aws_lambda_powertools/utilities/parameters/ssm.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0777858...cec2f33. Read the comment docs.

ran-isenberg · 2022-04-06T18:19:09Z

not really sure why codecov doesnt show an updated version. I added tests

heitorlessa · 2022-04-08T07:40:20Z

thanks for that @ran-isenberg - fixed the title as I see this as a bug as customers can't use the functionality we provide.

The pull request body is missing our required acknowledgements: are you okay if I edit the content to add them back as I can't review PRs without it?

Thanks a lot!

ran-isenberg · 2022-04-08T07:42:46Z

@heitorlessa after all this time, do you really need to ask? :)
go for it

heitorlessa

Two minor changes - 1/ Make use of forward reference for type annotation ( "SSMClient" from mypy_boto3_<service), and 2/ refactor branching logic into a static method to prevent logic discrepancy in a future change.

I ran out of time to look into the tests and DynamoDB, but I will as soon as I'm able.

Side note: Secrets Manager and SSM seem to be the only ones supporting endpoint_url according to this doc, yet boto3_client will benefit everyone's testing: https://docs.aws.amazon.com/vpc/latest/privatelink/integrated-services-vpce-list.html

docs/utilities/parameters.md

aws_lambda_powertools/utilities/parameters/ssm.py

aws_lambda_powertools/utilities/parameters/secrets.py

aws_lambda_powertools/utilities/parameters/appconfig.py

heitorlessa · 2022-04-08T08:54:43Z

aws_lambda_powertools/utilities/parameters/appconfig.py

+        if boto3_client is not None:
+            self.client = boto3_client
+        else:
+            session = boto3_session or boto3.session.Session()
+            self.client = session.client("appconfig", config=config)
+


Can we turn this into a builder static method in the parent class? Return type could be Any as we will be able to cast the correct one for each client built w/ mypy_boto3.

This will prevent future an accidental logic discrepancy between them and localize change. It could accept the boto3_client, boto3_session, boto3_config, and boto3_service_name to initialize.

Let me know if it doesn't make sense

i didnt add it to the dymamo class becuase we dont use a client there, not usre you can create a table from a client class.
so if it's not the dynamodb, it shouldn't be in the parent

michaelbrewer · 2022-04-10T23:44:58Z

Two minor changes - 1/ Make use of forward reference for type annotation ( "SSMClient" from mypy_boto3_<service), and 2/ refactor branching logic into a static method to prevent logic discrepancy in a future change.

I ran out of time to look into the tests and DynamoDB, but I will as soon as I'm able.

Side note: Secrets Manager and SSM seem to be the only ones supporting endpoint_url according to this doc, yet boto3_client will benefit everyone's testing: docs.aws.amazon.com/vpc/latest/privatelink/integrated-services-vpce-list.html

@heitorlessa @ran-isenberg - should we be bundling boto3-stubs as part of the python runtime? self.client is typed as Any and so it only benefits internal usage and not external. And internally we just explode the sdk_options into the method. So very little benefit. It would be nice to keep this light before splitting up modules.

heitorlessa · 2022-04-13T17:55:06Z

Haven't forgotten @ran-isenberg. I'll look into as soon as I can this week.

michaelbrewer · 2022-04-13T18:04:37Z

When this is released as is and it has to include a new runtime dependency for internal typing.

Can we include a warning in the changelog for customers like me at FiServ, who needs to scan for upstream issues and resolve conflicts if using a conflicting boto stubs.

Or change this make this optional dependency

michaelbrewer · 2022-04-13T18:56:05Z

maybe 2 PRs

154 lines to fix just feat(parameters): Allow settings boto3.client() arguments #1079
430 lines or more for the mypy stubs and what ever other packages could use it. Like Idempotency, Batch, Parameters, Data Class (code_pipeline_job_event)

heitorlessa

@ran-isenberg thanks for making those changes. I've made code change suggestions along with explanations about why they're needed - let me know if they don't make sense.

For the DynamoDB and parent static method piece, GitHub seems to have swallowed the review thread and I can't make comments anymore. I understand you concern about having a static method in the parent that's applicable to two clients (SSM, SecretsManager) but DynamoDB. I'd like to propose a solution to unblock this PR, as we still need to address DynamoDB regardless.

Suggestion: Add the same boto3_client in DynamoDB with an explicit resource type definition, plus doc strings as well as docs stating that we expect a DynamoDB resource client. I thought about boto3_resource_client but ultimately it will come down to practicality at this point, hence the suggestion of keeping boto3_client, as resource is also a client (high level vs low level).

aws_lambda_powertools/utilities/parameters/appconfig.py

aws_lambda_powertools/utilities/parameters/secrets.py

aws_lambda_powertools/utilities/parameters/ssm.py

docs/utilities/parameters.md

pyproject.toml

michaelbrewer · 2022-04-15T07:29:36Z

@ran-isenberg thanks for making those changes. I've made code change suggestions along with explanations about why they're needed - let me know if they don't make sense.

For the DynamoDB and parent static method piece, GitHub seems to have swallowed the review thread and I can't make comments anymore. I understand you concern about having a static method in the parent that's applicable to two clients (SSM, SecretsManager) but DynamoDB. I'd like to propose a solution to unblock this PR, as we still need to address DynamoDB regardless.

Suggestion: Add the same boto3_client in DynamoDB with an explicit resource type definition, plus doc strings as well as docs stating that we expect a DynamoDB resource client. I thought about boto3_resource_client but ultimately it will come down to practicality at this point, hence the suggestion of keeping boto3_client, as resource is also a client (high level vs low level).

Great work in finding an alternative of doing typing checking during development time and not make a required runtime dependency.

But does this make sense as part of this pull request? At a minimum just the added client parameter is needed to resolve #1079

heitorlessa · 2022-05-18T14:54:49Z

@heitorlessa @michaelbrewer I pushed the changes you wanted. I didnt do the dynamodb changes as I dont fully understand what you suggested. DynamoDB already gets an optional boto session, so not really sure what the client has to do with anything. We create a table from the session.
Feel free to make the changes yourself or explain them again :)

Thanks a lot for making those changes, Ran! I'll push the other changes and ping you for review later this week

As promised, I've transferred what I had in my local branch and made additional fixes. This is now ready for review.

@gshpychka - @sthulb will review and once approved we should release it ASAP to unblock the VPC private endpoints. We had a delay in getting this out to you due to a larger operational and internal work.

michaelbrewer · 2022-05-18T15:01:11Z

@heitorlessa @michaelbrewer I pushed the changes you wanted. I didnt do the dynamodb changes as I dont fully understand what you suggested. DynamoDB already gets an optional boto session, so not really sure what the client has to do with anything. We create a table from the session.
Feel free to make the changes yourself or explain them again :)

Thanks a lot for making those changes, Ran! I'll push the other changes and ping you for review later this week

As promised, I've transferred what I had in my local branch and made additional fixes. This is now ready to be review.

@gshpychka - @sthulb will review and once approved we should release it ASAP to unblock the VPC private endpoints. We had a delay in getting this out to you due to a larger operational and internal work.

Would this be a major release update? Have you done integration testing (due to the large number of changes)

michaelbrewer · 2022-05-18T15:26:43Z

aws_lambda_powertools/utilities/parameters/dynamodb.py

@@ -152,15 +158,19 @@ def __init__(
        endpoint_url: Optional[str] = None,
        config: Optional[Config] = None,
        boto3_session: Optional[boto3.session.Session] = None,
+        boto3_client: Optional["DynamoDBServiceResource"] = None,


For this kind of change docs with examples should also be added to avoid confusion between using client vs resource.

note, the original request was for appconfig.

As a customer without clarification this would be confusing and not everyone uses mypy types

Agreed on the docs, I will make those changes tomorrow (docs, not param name) and merge it.

thanks clarity always helps.

overall for DynamoDBProvider the resource client so that you can set the endpoint url seems redundant when you already have endpoint_url as a parameter.

With 8 parameters now, this is borderline code smell.

@heitorlessa the docs definitely helps! It might be a good general call out to recommend people use boto3 session where possible

sthulb

This PR looks fine to me.

My only qualm is about the order of session and clients in the providers, but given it's new functionality, it can be ignored

aws_lambda_powertools/utilities/parameters/appconfig.py

docs/utilities/parameters.md

sthulb · 2022-05-18T16:47:24Z

Would this be a major release update? Have you done integration testing (due to the large number of changes)

As per semver, this would require a minor release. 1.x.0, this doesn't remove any functionality or fundamentally change anything for users.

aws_lambda_powertools/utilities/parameters/appconfig.py

michaelbrewer · 2022-05-18T17:18:54Z

Would this be a major release update? Have you done integration testing (due to the large number of changes)

As per semver, this would require a minor release. 1.x.0, this doesn't remove any functionality or fundamentally change anything for users.

so a feature release ie: 1.26.0. I never expect this to be a 2.0.0 release.

aws_lambda_powertools/utilities/parameters/dynamodb.py

* develop: chore: bump to 1.26.0 chore(deps-dev): bump mypy-boto3-secretsmanager from 1.21.34 to 1.23.0.post1 (aws-powertools#1218) chore(deps-dev): bump mypy-boto3-appconfig from 1.21.34 to 1.23.0.post1 (aws-powertools#1219) chore(deps-dev): bump mypy-boto3-ssm from 1.21.34 to 1.23.0.post1 (aws-powertools#1220) chore(deps): bump pydantic from 1.9.0 to 1.9.1 (aws-powertools#1221) feat(parameters): accept boto3_client to support private endpoints and ease testing (aws-powertools#1096) fix(docs): remove Slack link (aws-powertools#1210)

feat(parameters): Allow settings boto3.client() arguments

ce851e9

boring-cyborg bot added area/utilities documentation Improvements or additions to documentation labels Apr 6, 2022

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 6, 2022

ran-isenberg mentioned this pull request Apr 6, 2022

feat(parameters): Allow settings boto3.client() arguments #1079

Closed

github-actions bot added the feature New feature or functionality label Apr 6, 2022

added tests

b9b6753

boring-cyborg bot added the tests label Apr 6, 2022

added tests

159ca22

heitorlessa changed the title ~~feat(parameters): Allow settings boto3.client() arguments~~ fix(parameters): Allow settings boto3.client() arguments Apr 8, 2022

github-actions bot added the bug Something isn't working label Apr 8, 2022

heitorlessa suggested changes Apr 8, 2022

View reviewed changes

heitorlessa and others added 2 commits April 8, 2022 10:59

chore: paying older debts on grammar

d33201e

added mypy type hints

2fbdca3

pull-request-size bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 10, 2022

boring-cyborg bot added the dependencies Pull requests that update a dependency file label Apr 10, 2022

pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 10, 2022

heitorlessa linked an issue Apr 12, 2022 that may be closed by this pull request

feat(parameters): Allow settings boto3.client() arguments #1079

Closed

heitorlessa suggested changes Apr 15, 2022

View reviewed changes

heitorlessa removed the feature New feature or functionality label Apr 15, 2022

merge main

ac5554f

heitorlessa added 5 commits May 18, 2022 15:09

chore: encapsulate client building logic in staticmethod

9b99f0b

feat(parameters): add boto3_client for dynamodb resource client

d2c8f30

chore: shorten docstrings, fix union single typehint

d854f1c

chore: ignore mypy ret-type/misc caught by new typed client

63bdde6

docs: add note on boto3_client usefulness for searching

3528b8e

heitorlessa changed the title ~~fix(parameters): accept boto3_client to support private endpoints~~ feat(parameters): accept boto3_client to support private endpoints May 18, 2022

heitorlessa removed the bug Something isn't working label May 18, 2022

github-actions bot added the feature New feature or functionality label May 18, 2022

chore: tests for dynamodb custom boto3 client

579f8e1

pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2022

heitorlessa added 2 commits May 18, 2022 16:42

chore: skip test coverage at runtime type_checking

a8350ae

chore: re-add boto config fixture custom client test

cec2f33

heitorlessa approved these changes May 18, 2022

View reviewed changes

heitorlessa requested a review from sthulb May 18, 2022 14:55

michaelbrewer suggested changes May 18, 2022

View reviewed changes

sthulb approved these changes May 18, 2022

View reviewed changes

aws_lambda_powertools/utilities/parameters/appconfig.py Show resolved Hide resolved

docs/utilities/parameters.md Show resolved Hide resolved

michaelbrewer reviewed May 18, 2022

View reviewed changes

aws_lambda_powertools/utilities/parameters/appconfig.py Show resolved Hide resolved

michaelbrewer reviewed May 18, 2022

View reviewed changes

aws_lambda_powertools/utilities/parameters/dynamodb.py Outdated Show resolved Hide resolved

heitorlessa added 3 commits May 19, 2022 08:34

chore: remove super leftover

19ec9ae

chore: final adjustments

58e2d5e

chore: correct leftover table client

98dc539

heitorlessa changed the title ~~feat(parameters): accept boto3_client to support private endpoints~~ feat(parameters): accept boto3_client to support private endpoints and ease testing May 19, 2022

heitorlessa merged commit 45867fc into aws-powertools:develop May 19, 2022

ran-isenberg deleted the boto branch October 23, 2022 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parameters): accept boto3_client to support private endpoints and ease testing #1096

feat(parameters): accept boto3_client to support private endpoints and ease testing #1096

ran-isenberg commented Apr 6, 2022 •

edited by heitorlessa

Loading

codecov-commenter commented Apr 6, 2022 •

edited

Loading

ran-isenberg commented Apr 6, 2022

heitorlessa commented Apr 8, 2022

ran-isenberg commented Apr 8, 2022

heitorlessa left a comment

heitorlessa Apr 8, 2022

ran-isenberg Apr 10, 2022

michaelbrewer commented Apr 10, 2022 •

edited

Loading

heitorlessa commented Apr 13, 2022

michaelbrewer commented Apr 13, 2022

michaelbrewer commented Apr 13, 2022

heitorlessa left a comment

michaelbrewer commented Apr 15, 2022

heitorlessa commented May 18, 2022 •

edited

Loading

michaelbrewer commented May 18, 2022

This comment was marked as resolved.

michaelbrewer May 18, 2022

michaelbrewer May 18, 2022

heitorlessa May 18, 2022

michaelbrewer May 18, 2022

michaelbrewer May 18, 2022

michaelbrewer May 19, 2022

sthulb left a comment

sthulb commented May 18, 2022

michaelbrewer commented May 18, 2022 •

edited

Loading

feat(parameters): accept boto3_client to support private endpoints and ease testing #1096

feat(parameters): accept boto3_client to support private endpoints and ease testing #1096

Conversation

ran-isenberg commented Apr 6, 2022 • edited by heitorlessa Loading

Summary

Changes

User experience

Checklist

Acknowledgment

codecov-commenter commented Apr 6, 2022 • edited Loading

Codecov Report

ran-isenberg commented Apr 6, 2022

heitorlessa commented Apr 8, 2022

ran-isenberg commented Apr 8, 2022

heitorlessa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbrewer commented Apr 10, 2022 • edited Loading

heitorlessa commented Apr 13, 2022

michaelbrewer commented Apr 13, 2022

michaelbrewer commented Apr 13, 2022

heitorlessa left a comment

Choose a reason for hiding this comment

michaelbrewer commented Apr 15, 2022

heitorlessa commented May 18, 2022 • edited Loading

michaelbrewer commented May 18, 2022

This comment was marked as resolved.

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

sthulb left a comment

Choose a reason for hiding this comment

sthulb commented May 18, 2022

michaelbrewer commented May 18, 2022 • edited Loading

ran-isenberg commented Apr 6, 2022 •

edited by heitorlessa

Loading

codecov-commenter commented Apr 6, 2022 •

edited

Loading

michaelbrewer commented Apr 10, 2022 •

edited

Loading

heitorlessa commented May 18, 2022 •

edited

Loading

michaelbrewer commented May 18, 2022 •

edited

Loading