-
Notifications
You must be signed in to change notification settings - Fork 81
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
Unify Logger Config for Tasks #1709
Conversation
Testing:
Run ECS Tasks and Verify Proper Logs + Formatting w/ Log Level -
Run ECS Tasks and Verify Proper Logs + Formatting w/ Log Level -
|
import os | ||
import sys | ||
|
||
logging.basicConfig( |
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 I understand correctly this statement here will affect all data.all backend services (lambda, ecs tasks etc).
Should we be removing ALL the per file logging
configs? With a quip grep I see 206 files
grep -rail "logging.getLogger(" backend/ | wc -l
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.
Do we? I thought that the getlogger is just getting the logger, not configuring it
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.
You are correct it affects every file under /backend/dataall/
- I though this would simplify how we were formerly managing logs in each individual location
It also solves the issue we have where a task (like share manager task - at dataall/backend/dataall/modules/shares_base/tasks/share_manager_task.py
) was not recording the logs from SharingService (at dataall/backend/dataall/modules/shares_base/services/sharing_service.py
) or the Processors and we were missing logs in CloudWatch
For Reference in dataall-sbx-backend-graphql
the log group formatting before the change:
[INFO] 2024-11-19T15:28:43.058Z a725615b-f975-4a66-8ccb-7e69830f18b9 Current maintenance window status - INACTIVE
[INFO] 2024-11-19T15:28:43.060Z a725615b-f975-4a66-8ccb-7e69830f18b9 SSM Parameter session in central account
And after the code change (same formatting):
[INFO] 2024-11-20T22:34:02.450Z 2133b99e-3316-44de-9b9c-68f02f96611c Current maintenance window status - INACTIVE
[INFO] 2024-11-20T22:34:02.452Z 2133b99e-3316-44de-9b9c-68f02f96611c SSM Parameter session in central account
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.
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.
@dlpzx we do configure at a lot of places
@noah-paige I am fine if you want to refactor and remove the configs from all the files keeping only the top one
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.
For a full breakdown of where we call logger.setLevel(...)
which I think is the main config we do in a number of places (was ~26 places before this PR now at 15 files):
- Backend Files not under
dataall/backend/dataall/
(5):- dataall/backend/api_handler.py
- dataall/backend/aws_handler.py
- dataall/backend/local_graphql_server.py
- dataall/backend/search_handler.py
- For rest of Backend Code:
- dataall/backend/dataall/init.py
- For CDK specific activity:
- dataall/backend/dataall/base/cdkproxy/app.py (creates new logger named
cdkapp process
always with level INFO) -- chose to leave as is
- dataall/backend/dataall/base/cdkproxy/app.py (creates new logger named
- Lambdas from data.all resource Custom Resource CDK
- dataall/backend/dataall/modules/s3_datasets/cdk/assets/gluedatabasecustomresource/index.py
- dataall/backend/dataall/modules/s3_datasets/cdk/assets/lakeformationdefaultsettings/index.py
- Trigger Function Lambdas data.all pipeline
- dataall/backend/deployment_triggers/dbmigrations_handler.py
- dataall/backend/deployment_triggers/dbsnapshots_handler.py
- dataall/backend/deployment_triggers/saveperms_handler.py
- Custom Resource Lambdas data.all Deployment
- dataall/deploy/custom_resources/cognito_config/cognito_urls.py
- dataall/deploy/custom_resources/cognito_config/cognito_users.py
- dataall/deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py
- dataall/deploy/custom_resources/custom_authorizer/jwt_services.py
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.
@dlpzx @petrkalos - the places where we configure logs is more intentional now and for the majority of backend it is from dataall/backend/dataall/__init__.py
which I think is best
Some that are in different compute functions or parts of deployment will remain separate
<!-- please choose --> - Refactoring - Unify Logger Config in Backend (focused on `/tasks`) - Fix Log Level setting - #1680 - #1662 Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
### Feature or Bugfix - Security ### Detail ### 🔐 Security * Update sanitization technique for terms filtering by @noah-paige in #1692 and in #1693 * Move access logging to a separate environment logging bucket by @noah-paige in #1695 * Add explicit token duration config for both JWTs by @noah-paige in #1698 * Disable GraphQL introspection if prod sizing by @noah-paige in #1704 * Add snyk workflow on schedule by @noah-paige in #1705, #1708, #1713, #1745 and in in #1746 * Unify Logger Config for Tasks by @noah-paige in #1709 * Updating overly permissive policies tagged by checkov for environment role using least privilege principles by @mourya-33 in #1632 Data.all permission model has been reviewed to ensure all Mutations and Queries have proper permissions: * Add MANAGE_SHARES permissions by @dlpzx in #1702 * Add permission check - is tenant to update SSM parameters API by @dlpzx in #1714 * Add GET_SHARE_OBJECT permissions to get data filters API by @dlpzx in #1717 * Add permissions on list datasets for env group + cosmetic S3 Datasets by @dlpzx in #1718 * Add GET_WORKSHEET permission in RUN_SQL_QUERY by @dlpzx in #1716 * Add permissions to Quicksight monitoring service layer by @dlpzx in #1715 * Add LIST_ENVIRONMENT_DATASETS permission for listing shared datasets and cleanup unused code by @dlpzx in #1719 * Add is_owner permissions to Glossary mutations + add new integration tests by @dlpzx in #1721 * Refactor env permissions + modify getTrustAccount by @dlpzx in #1712 * Add Feed consistent permissions by @dlpzx in #1722 * Add Votes consistent permissions by @dlpzx in #1724 * Consistent get_<DATA_ASSET> permissions - Dashboards by @dlpzx in #1729 ### 🧪 Test improvements Integration tests are in sync with `main` without 2.7 planned features. In this PR all core modules, optional modules and submodules are tested. That includes: tenant-permissions, omics, mlstudio, votes, notifications and backwards compatiblity of s3 shares. by @SofiaSazonova, @noah-paige , @petrkalos and @dlpzx In addition, the following PR adds functional tests that ensure the permission model of data.all is not corrupted. * ⭐ Add resource permission checks by @petrkalos in #1711 ### Dependencies * Update FastAPI by @petrkalos in #1577 * update fastapi dependency by @noah-paige in #1699 * Upgrade "cross-spawn" to "7.0.5" by @dlpzx in #1701 * Bump python runtime to bump cdk klayers cryptography version by @noah-paige in #1707 ### Relates - List above ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: mourya-33 <[email protected]> Co-authored-by: Mourya Darivemula <[email protected]> Co-authored-by: Noah Paige <[email protected]> Co-authored-by: Petros Kalos <[email protected]> Co-authored-by: Sofia Sazonova <[email protected]> Co-authored-by: Sofia Sazonova <[email protected]>
Feature or Bugfix
Detail
/tasks
)Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.