Skip to content
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

feat(parameters): add get_parameters_by_name for SSM params in distinct paths #1678

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Nov 2, 2022

Issue number: #1040

Summary

Today, customers can fetch multiple SSM parameters recursively by using a path. This happens server-side.

As described in the issue, customers may need to fetch multiple parameters in distinct paths. As of now, this requires additional boilerplate to get it done properly.

Changes

Please provide a summary of what's being changed

This PR introduces get_parameter_by_name high-level function that accepts a dictionary containing:

  • A parameter to fetch
  • A dict optionally containing overrides for a parameter (e.g., max_age, transform, decrypt, etc.)

Non-related but necessary changes

  • Adds typing-extensions as a dependency to provide strict typing for this feature. Depending on the transform value, the return type is precise.
  • Refactored transform_method and transform_value to better support strict typing and related bugs
  • Created add_to_cache method to support refactoring and handle max_age=0, max_age=-1 scenarios we didn't cover before
  • Exposed has_not_expired_in_cache method to ease testing

User experience

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

from typing import Any

from aws_lambda_powertools.utilities import get_parameters_by_name

parameters = {
  "/develop/service/commons/telemetry/config": {"max_age": 300, "transform": "json"},
  "/no_cache_param": {"max_age": 0},
  # inherit default values
  "/develop/service/payment/api/capture/url": {},
}

def handler(event, context):
    # This returns a dict with the parameter name as key
    response: dict[str, Any] = parameters.get_parameters_by_name(parameters=parameters, max_age=60)
    for parameter, value in response.items():
        print(f"{parameter}: {value}")

Overrides fail-fast to support graceful error handling

from typing import Any

from aws_lambda_powertools.utilities import get_parameters_by_name

parameters = {
  "/develop/service/commons/telemetry/config": {"max_age": 300, "transform": "json"},
  # it would fail by default
  "/this/param/does/not/exist"
}

def handler(event, context):
    values: dict[str, Any] = parameters.get_parameters_by_name(parameters=parameters, raise_on_error=False)
    errors: list[str] = values.get("_errors", [])

    # Handle gracefully, since '/this/param/does/not/exist' will only be available in `_errors`
    if errors:
        ...

    for parameter, value in values.items():
        print(f"{parameter}: {value}")

Checklist

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

  • Meet tenets criteria
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented
  • PR title follows conventional commit semantics
  • Test whether threads can be faster or not (Nope due to CPU slicing in Lambda + LWP)
  • Test whether async can be faster or not (Nope due to event loop setup + aioboto need)
  • Use GetParameters as default. Use GetParameter when decrypt is set on a per parameter
  • Functional test for options override
  • Functional test for partial failure
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.

@heitorlessa heitorlessa requested a review from a team as a code owner November 2, 2022 09:27
@heitorlessa heitorlessa requested review from leandrodamascena and removed request for a team November 2, 2022 09:27
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2022
@boring-cyborg boring-cyborg bot added the dependencies Pull requests that update a dependency file label Nov 2, 2022
@heitorlessa heitorlessa marked this pull request as draft November 2, 2022 09:27
@github-actions github-actions bot added the feature New feature or functionality label Nov 2, 2022
@heitorlessa
Copy link
Contributor Author

cc @peterschutt as this is the first PR that adds support for typing-extensions to make use of Literal for strict typing.

@heitorlessa
Copy link
Contributor Author

heitorlessa commented Nov 2, 2022

Two main questions to discuss in the original issue:

  1. Should we keep parallel= parameter when single thread is faster* in this context?
  2. What should be the error strategy?
    • (A) Treat as non-atomic by catching the error, and include each failed param in a errors key along with the reason.
    • (B) Fail-fast by default with the option to override (raise_on_failure). When overridden, it falls back to A
    • (C) Something else

Did two tests within Lambda:

  1. 20 params to fetch leading to a timeout with parallel=True due to SSM throttling leading to exponential backoff even w/ 30s timeout
  2. 10 params to fetch

Quick results

  • First trace: 20 params in single and multi-threading with 128M memory size. The errors were due to throttling leading to exponential back-off to the point the operation timed out. Successful ones were with 10 params (no throttling).

  • Second trace: Same 10 params but using with 1024M memory size

Without much further digging, this is largely due to CPU slicing in Lambda (one real core only at ~1.7G), combined with LWP cost and SSM throttling.

image

image

Signed-off-by: heitorlessa <lessa@amazon.co.uk>
@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Nov 2, 2022
@boring-cyborg boring-cyborg bot added the tests label Nov 2, 2022
@jplock
Copy link

jplock commented Nov 2, 2022

  • I could see this being abused by trying to fetch too many parameters, so if less than 10 keys to fetch, fetch serially, if more than 10, parallelize into batches of 10?
  • I like option B with the raise_on_failure option
  • From a usage standpoint, are people wanting to fetch separate specific keys like this, or do they really want to do fetch all child keys under a parent (like /develop/service/*)?

@heitorlessa
Copy link
Contributor Author

Thanks @jplock. Great to hear from you as usual.

For the first point, that's what we're gonna do with a small difference - if one enables decrypt as a default Or per parameter, we need to use GetParameter. Doing the latter in parallel can lead to timeouts easily, will do serially for decrypt calls instead.

From a usage standpoint, are people wanting to fetch separate specific keys like this, or do they really want to do fetch all child keys under a parent (like /develop/service/*)?

The former. We already have a solution for the latter via get_parameters(path="/develop/service")

@pull-request-size pull-request-size bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 3, 2022
@boring-cyborg boring-cyborg bot added the commons label Nov 3, 2022
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 3, 2022
@heitorlessa
Copy link
Contributor Author

another TIL

Service model for GetParameters response requires a non-empty value for InvalidParameters even for a successful request - this makes functional testing with stubs impossible.

Need to create an issue in Boto repo. Actual API returns an empty list.

    ...
    stubber = stub.Stubber(provider.client)
    response = {
        "Parameters": [
            {
                "Name": dev_param,
                "Type": "String",
                "Value": "string",
                "Version": mock_version,
                "Selector": f"{dev_param}:{mock_version}",
                "SourceResult": "string",
                "LastModifiedDate": datetime(2015, 1, 1),
                "ARN": f"arn:aws:ssm:us-east-2:111122223333:parameter/{dev_param.removeprefix('/')}",
                "DataType": "string",
            },
            {
                "Name": prod_param,
                "Type": "String",
                "Value": "string",
                "Version": mock_version,
                "Selector": f"{prod_param}:{mock_version}",
                "SourceResult": "string",
                "LastModifiedDate": datetime(2015, 1, 1),
                "ARN": f"arn:aws:ssm:us-east-2:111122223333:parameter/{prod_param.removeprefix('/')}",
                "DataType": "string",
            },
        ],
        "InvalidParameters": [],  # NOTE: stub fails on validation despite not having any
    }

    expected_params = {"Names": param_names}
    stubber.add_response("get_parameters", response, expected_params)  # fail here
    stubber.activate()

Validation error when adding response

E           botocore.exceptions.ParamValidationError: Parameter validation failed:
E           Invalid length for parameter InvalidParameters, value: 0, valid min length: 1

@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 7, 2022
@heitorlessa heitorlessa marked this pull request as ready for review November 7, 2022 11:24
@heitorlessa
Copy link
Contributor Author

ASCII diagram to ease understanding of how we decide when to use one or both APIs to fetch parameters.

                           ┌────────────────────────┐
                       ┌───▶  Decrypt entire batch  │─────┐
                       │   └────────────────────────┘     │     ┌────────────────────┐
                       │                                  ├─────▶ GetParameters API  │
┌──────────────────┐   │   ┌────────────────────────┐     │     └────────────────────┘
│   Split batch    │───┼──▶│ No decryption required │─────┘
└──────────────────┘   │   └────────────────────────┘
                       │                                        ┌────────────────────┐
                       │   ┌────────────────────────┐           │  GetParameter API  │
                       └──▶│Decrypt some but not all│──────────▶├────────────────────┤
                           └────────────────────────┘           │ GetParameters API  │
                                                                └────────────────────┘

@codecov-commenter
Copy link

Codecov Report

Base: 99.34% // Head: 99.27% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (9e84a07) compared to base (635bc80).
Patch coverage: 97.07% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1678      +/-   ##
===========================================
- Coverage    99.34%   99.27%   -0.07%     
===========================================
  Files          128      129       +1     
  Lines         5917     6050     +133     
  Branches       377      400      +23     
===========================================
+ Hits          5878     6006     +128     
- Misses          18       20       +2     
- Partials        21       24       +3     
Impacted Files Coverage Δ
...da_powertools/utilities/feature_flags/appconfig.py 100.00% <ø> (ø)
aws_lambda_powertools/utilities/parameters/ssm.py 97.40% <96.55%> (-2.60%) ⬇️
aws_lambda_powertools/utilities/parameters/base.py 99.07% <97.82%> (-0.93%) ⬇️
aws_lambda_powertools/shared/functions.py 95.65% <100.00%> (+0.41%) ⬆️
...lambda_powertools/utilities/parameters/__init__.py 100.00% <100.00%> (ø)
...ambda_powertools/utilities/parameters/appconfig.py 94.44% <100.00%> (+0.15%) ⬆️
...ws_lambda_powertools/utilities/parameters/types.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@heitorlessa
Copy link
Contributor Author

heitorlessa commented Nov 7, 2022

Merging after extensive tests and self-review - as Ruben and Leandro are on PTO

This PR made it clear that we have to prioritize refactoring Parameters entirely. Hit the following issues which delayed completion by nearly a week:

  • GetParameters can decrypt all or no parameters
  • GetParameters call is equal a single API call which makes it safer for API Throttling
  • GetParameters is non-atomic which means errors are aggregated under InvalidParameters
  • GetParameters can fail atomically when KMS key is invalid (or HTTP 500)
  • Lacks of good typing revealed bugs in value transformation (None, auto, Dict values)
  • SSMProvider and others have test side effects due to boto3 client initialization in super class when testing
  • SSM also allows parameters without / which means we need to safeguard against _errors in case they're present when fetching parameters
  • Boto stub for GetParameters incorrectly fails when InvalidParameters is an empty list. This meant making tests harder or artificially make an invalid param to support SDK data validation
  • We didn't expose adding to cache and verifying whether param was in cache, requiring additional refactoring
  • We were unnecessarily caching parameters with max_age null or negative
  • We need a bunch of overloads due to the flexibility of BaseProvider, thus requiring type-extensions to get to an acceptable level of accuracy (excluding transform_value)
  • Our BaseProvider nomenclature for get and _get like methods make it harder to implement complex logic like get_parameters_by_name which requires multiple levels of iterables

@heitorlessa heitorlessa removed the request for review from leandrodamascena November 7, 2022 16:22
@heitorlessa heitorlessa merged commit e9c9516 into aws-powertools:develop Nov 7, 2022
@heitorlessa heitorlessa deleted the feat/get-parameters-by-name branch November 7, 2022 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commons dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation feature New feature or functionality size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants