-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
update api-dependencies and check cloudwatch/logs enabled #9576
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
Conversation
| ) | ||
| if self_managed_endpoints.get("KAFKA_BOOTSTRAP_SERVERS"): | ||
| service_type = "kafka" | ||
| elif not is_api_enabled(service_type): |
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.
did not add the this logic to the start_listeners as the function will be removed for v3
| ) | ||
|
|
||
| def start_subscriber(self) -> None: | ||
| if not is_api_enabled("logs"): |
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.
just preventing the thread start here. we can discuss if there are better approaches to handle this when logs are disabled.
bentsku
left a comment
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.
LGTM! This is awesome! Also, great catch for sts. 🚀
| "es": ["opensearch"], | ||
| "lambda": ["logs", "cloudwatch", "s3", "sqs"], | ||
| "kinesis": ["dynamodb"], | ||
| "lambda": ["s3", "sqs", "sts"], |
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.
good catch! 👍
7dad74e to
9353f57
Compare
9353f57 to
46c2cc9
Compare
joe4dev
left a comment
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 PR covers the basic Lambda scenarios 👍
It's gonna be tough to maintain these conditionals for many service combinations.
I tested the provided testing scenario with SERVICES=lambda and this PR fixes Lambda create-function, invoke, and (dummy) event source mapping (already create-function would fail without this PR) 👏👏👏 .
event_manager.py 🤔 Would it provide some helpful logging output?
I guess one of the relevant scenarios for 3.0 is related to users who have set SERVICES since a long time before it got re-purposed. Obviously, any user code depending on other services within Lambda could fail somewhat implicitly but this likely falls under misconfiguration.
Out of the scope for this PR but I just wanted to comment on integrations to consider for ext:
- X-Ray
- and potentially ECR for pulling Lambda
- IAM
| if self_managed_endpoints.get("KAFKA_BOOTSTRAP_SERVERS"): | ||
| service_type = "kafka" | ||
| elif not is_api_enabled(service_type): | ||
| LOG.info( |
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 info sufficient here or should this rather be a warning given the functionality just does not work and it might be a misconfiguration?
Maybe the question is how often is it a misconfiguration vs. intentional disabling 🤷
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.
IMO info should be fine, because it's impossible to pass a valid/existing source-arn here (otherwise the service would be available).
Generally for every access of a service that is disabled, we would throw a
Yes, they should run into issues quite fast though. We expect to get those kind of reports, for most customers removing the |
Motivation
Previous PR #9494 enabled the loading of specific services only.
In this PR we update the
API_DEPENDENCIESfor community, and added checks for before calling non-required services.Changes
Lambda
EventSourceListenerto start if the event-source service is not loadedLogHandlerthread, iflogsis not enabledec2is not enabledCloudWatch-Util
publish_lambda_metricis also used bylambdaLogs
cloudwatchis disabledupdated
API_DEPENDENCIESkinesis: removed dependency ondynamodbas it doesn't seem to be calledlambda: removedcloudwatch+logs, addedstsTesting
Automated testing of this fix is hard, mainly because also for bootstrapped-test we cannot invoke the lambda.
As a manual check:
Before it would keep logging errors:
With this changes, we can add a mocked event-source-mapping without any errors, also invoke the lambda,
logsandcloudwatchwill not be enabled, and there are also no errors in the localstack logs.