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.

Sign up for GitHub

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

Jump to bottom

log API handler response only for LOG_LEVEL DEBUG. Set log level INFO for prod deployments #1662

Merged
merged 2 commits into from
Oct 28, 2024
Merged

log API handler response only for LOG_LEVEL DEBUG. Set log level INFO for prod deployments #1662

merged 2 commits into from
Oct 28, 2024

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Oct 25, 2024 •
edited
Loading

Feature or Bugfix

  • Feature

Detail

  • Define LOG_LEVEL based on prod_sizing parameter: for prod deployments LOG_LEVEL=INFO, for non-prod deployments LOG_LEVEL=DEBUG. Set it as environment variable for backend Lambdas and ECS tasks
  • Remove any hardcoded setLevel and re-use the environment variable
  • For the API handler Lambda, log the response of the API call with level=DEBUG and log the success with level=INFO.

Testing

Deployed in AWS, CICD pipeline succeeds (with prod_sizing=False)

  • API calls work as usual - logs collect INFO (logs success) and DEBUG (logs lambda response)
  • ECS tasks work as usual - tested with CDK proxy call

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

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

@dlpzx dlpzx force-pushed the fix/remove-preview-data-logs branch from abfa63d to 6dfcd8e Compare October 25, 2024 09:49
@dlpzx dlpzx marked this pull request as ready for review October 25, 2024 12:04
SofiaSazonova
SofiaSazonova approved these changes Oct 28, 2024
View reviewed changes
Copy link
Contributor

@SofiaSazonova SofiaSazonova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor questions (maybe it's fine)
Also a question: maybe it makes sense to include in this PR the changes in logging? I think right now logging is inconsistent in terms of what to show as info and what as debug

backend/dataall/base/aws/quicksight.py
@@ -3,8 +3,7 @@

from .sts import SessionHelper

logger = logging.getLogger('QuicksightHandler')
logger.setLevel(logging.DEBUG)
logger = logging.getLogger(__name__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't here be
log.setLevel(os.environ.get('LOG_LEVEL', 'INFO'))
?

backend/dataall/modules/dashboards/aws/dashboard_quicksight_client.py
@@ -9,7 +9,6 @@
from dataall.base.aws.secrets_manager import SecretsManager

log = logging.getLogger(__name__)
log.setLevel(logging.DEBUG)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't here be
log.setLevel(os.environ.get('LOG_LEVEL', 'INFO'))
?

backend/dataall/modules/notifications/handlers/notifications_handler.py
@@ -5,7 +5,6 @@
from dataall.modules.notifications.services.ses_email_notification_service import SESEmailNotificationService

log = logging.getLogger(__name__)
log.setLevel(logging.INFO)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@noah-paige noah-paige merged commit 21ff8dc into main Oct 28, 2024
10 checks passed
@noah-paige noah-paige mentioned this pull request Nov 4, 2024
Closed
dlpzx added a commit that referenced this pull request Nov 6, 2024
@dlpzx
log API handler response only for LOG_LEVEL DEBUG. Set log level INFO…
1c7b771 
… for prod deployments (#1662)

- Feature

- Define LOG_LEVEL based on prod_sizing parameter: for prod deployments
LOG_LEVEL=INFO, for non-prod deployments LOG_LEVEL=DEBUG. Set it as
environment variable for backend Lambdas and ECS tasks
- Remove any hardcoded setLevel and re-use the environment variable
- For the API handler Lambda, log the response of the API call with
level=DEBUG and log the success with level=INFO.

Deployed in AWS, CICD pipeline succeeds (with prod_sizing=False)
- API calls work as usual - logs collect INFO (logs success) and DEBUG
(logs lambda response)
- ECS tasks work as usual - tested with CDK proxy call

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.
@dlpzx dlpzx mentioned this pull request Nov 6, 2024
Merged
dlpzx added a commit that referenced this pull request Nov 8, 2024
@dlpzx @noah-paige @petrkalos
2.6.1 Security features (#1686)
9f087ff 
### Feature or Bugfix
- Security

### Detail
* get-parameter CloudfrontDistributionDomainName from us-east-1 (#1687 )
* Added Token Validations (#1682)
* add warning to untrust data.all account when removing an environment
(#1685)
* add custom domain support for apigw (#1679)
* Lambda Event Logs Handling (#1678)
* Upgrade Spark version to 3.3 (#1675) -
a0c63a4
* ES Search Query Collect All Response  (#1631)
* Extend Tenant Perms Coverage (#1630)
* Limit Response info dataset queries (#1665)
* Add Removal Policy Retain to Bucket Policy IaC (#1660) 
* log API handler response only for LOG_LEVEL DEBUG. Set log level INFO
for prod deployments (#1662)
* Add permission checks to markNotificationAsRead + deleteNotification
(#1654)
* Added error view and unified utility to check tenant user (#1657
* Userguide signout flow (#1629)

### Relates
- Security release

### 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: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: Petros Kalos <kalosp@amazon.com>
@noah-paige noah-paige mentioned this pull request Nov 20, 2024
Merged
@dlpzx dlpzx deleted the fix/remove-preview-data-logs branch November 22, 2024 11:20
dlpzx pushed a commit that referenced this pull request Nov 26, 2024
@noah-paige
Unify Logger Config for Tasks (#1709)
d76d55c 
### Feature or Bugfix
<!-- please choose -->
- Refactoring

### Detail
-  Unify Logger Config in Backend (focused on `/tasks`)
-  Fix Log Level setting

### Relates
- #1680
- #1662
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SofiaSazonova SofiaSazonova SofiaSazonova approved these changes

Assignees

@dlpzx dlpzx

Labels
None yet
Projects
v2.7.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dlpzx @SofiaSazonova @noah-paige