[OSDEV-1886] Delete Lambda@Edge function before destroying environment#567
[OSDEV-1886] Delete Lambda@Edge function before destroying environment#567
Conversation
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 33.18%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 WalkthroughWalkthroughA new job named Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
.github/workflows/destroy.yml (1)
110-116: 🛠️ Refactor suggestion
⚠️ Potential issueEnforce Sequential Job Execution for Safe Environment Destruction
To fulfill the PR objective of deleting the Lambda@Edge function before destroying the associated infrastructure, ensure that thedestroyjob depends on the successful completion of thedestroy_lambda_edge_functionjob. Presently, thedestroyjob only listsclean_ecr_repositoriesas a dependency. Addingdestroy_lambda_edge_functionto itsneedslist will guarantee the proper order of resource teardown.
🧹 Nitpick comments (4)
.github/workflows/destroy.yml (4)
39-48: Ensure Consistent Environment Variable Usage and Input for Lambda Deletion Job
The new jobdestroy_lambda_edge_functioncorrectly introduces a step to obtain the environment name. However, please verify that the Lambda ARN construction uses the intended case—consider confirming that${{ vars.ENV_NAME }}(used for the Lambda name) is consistent with the lower-cased output from the change-string-case action if necessary. Also, ensure that all required variables (for example,CLOUDFRONT_DOMAIN) are defined in the workflow context or as repository secrets/environment variables.
51-54: Clarify Lambda ARN and Version Retrieval
The script retrieves the AWS account ID and constructs the Lambda ARN using the hardcoded region"us-east-1", which is appropriate for Lambda@Edge. However, the command that fetchesLAMBDA_VERSIONusingjq -r '.Versions | max_by(.Version) | .Version'assumes that at least one version exists and that the version comparison behaves as expected (especially regarding$LATEST). Consider adding error handling or a validation check in case this command returns an empty value or an unexpected result.
72-81: Enhance CloudFront Configuration Update Robustness
After fetching the distribution configuration, the script usesjqto delete the Lambda function association. While this implementation is functional, consider adding error handling to confirm that the JSON structure contains the expectedDefaultCacheBehavior.LambdaFunctionAssociations.Itemskey. Additionally, verifying that the update command (aws cloudfront update-distribution) succeeds before proceeding would improve reliability.
88-94: Review Lambda Versions Deletion Logic
The loop iterates over all Lambda function versions (retrieved viaaws lambda list-versions-by-function) and deletes those that are not equal to"$LATEST". Please verify that the comparison (if [[ "$version" != "\$LATEST" ]]; then) correctly skips the$LATESTversion, as escaping might behave unexpectedly. Additionally, consider logging or handling any errors returned by the deletion command for improved transparency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/destroy.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
29-29: Clarify and enhance documentation for Lambda@Edge deletionThis new entry for OSDEV-1886 clearly states that the Lambda@Edge function must be deleted before destroying infrastructure to avoid Terraform errors when attempting to delete a replicated function. The note is accurate and aligns with the PR objectives.
It would be beneficial to consider linking to any associated deployment or Terraform documentation (or automation workflows) that explain how this deletion is handled in practice. This additional context could help maintainers and developers understand the end-to-end process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roninzp
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
left a comment
vladsha-dev
left a comment
There was a problem hiding this comment.
- I have left comments about the implementation.
- Please add the reviewers to the PR.
- Please add the task number to the PR title like that [OSDEV-1886] Delete Lambda@Edge function before ....
- Please make the PR description basically the same as the release note.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
deployment/delete_lambda (4)
5-9: Missing validation for script argumentThe script assumes $1 is provided but lacks validation. Since you've set
-uflag, the script will fail if no argument is provided, but it would be better to include a proper validation with a helpful error message.#!/bin/bash set -euo pipefail +if [ $# -lt 1 ]; then + echo "Error: Environment name argument is required" + echo "Usage: $0 <environment_name>" + exit 1 +fi + ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text) REGION="us-east-1" ENV_NAME=$1 LAMBDA_ARN="arn:aws:lambda:$REGION:$ACCOUNT_ID:function:funcOpenSupplyHub${ENV_NAME}RedirectToS3origin" LAMBDA_VERSION=$(aws lambda list-versions-by-function --function-name "$LAMBDA_ARN" --region "$REGION" | jq -r '.Versions | max_by(.Version) | .Version')
32-36: Consider error handling for CloudFront updateThe script updates the CloudFront distribution but doesn't check if the update was successful. Consider adding error handling for this critical operation.
echo "Updating CloudFront distribution to detach Lambda@Edge..." UPDATED_CONFIG=$(echo "$CONFIG" | jq -c '.') aws cloudfront update-distribution --id "$DISTRIBUTION_ID" --if-match "$ETAG" --distribution-config "$UPDATED_CONFIG" + +if [ $? -ne 0 ]; then + echo "Error: Failed to update CloudFront distribution" + exit 1 +fi
43-49: Add rate limiting and error handling for Lambda deletionsWhen deleting multiple Lambda versions in a loop, consider adding a sleep to avoid hitting AWS API rate limits. Also, add error handling to check if each deletion was successful.
VERSIONS=$(aws lambda list-versions-by-function --function-name "$LAMBDA_ARN" --region "$REGION" | jq -r '.Versions[].Version') for version in $VERSIONS; do if [[ "$version" != "\$LATEST" ]]; then echo "Deleting Lambda version: $version" aws lambda delete-function --function-name "$LAMBDA_ARN:$version" --region "$REGION" + if [ $? -ne 0 ]; then + echo "Warning: Failed to delete Lambda version: $version" + fi + # Add a small sleep to avoid API rate limits + sleep 2 fi done
53-61: Add timeout to verification loopThe verification loop runs indefinitely until the Lambda function is deleted. Consider adding a timeout to avoid the script running forever if the deletion fails.
echo "Lambda function deletion initiated. Verifying..." +MAX_RETRIES=20 +RETRY_COUNT=0 while true; do + RETRY_COUNT=$((RETRY_COUNT + 1)) + if [ $RETRY_COUNT -gt $MAX_RETRIES ]; then + echo "Timeout: Lambda function deletion verification exceeded maximum retries" + exit 1 + fi if ! aws lambda get-function --function-name "$LAMBDA_ARN" --region "$REGION" 2>/dev/null; then echo "Lambda function successfully deleted." break fi echo "Waiting for Lambda function to be deleted..." sleep 30 done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/destroy.yml(1 hunks)deployment/delete_lambda(1 hunks)doc/release/RELEASE-NOTES.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/destroy.yml
- doc/release/RELEASE-NOTES.md
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
🔇 Additional comments (2)
deployment/delete_lambda (2)
1-3: Good practice with strict error handling!The script uses appropriate shell options (
set -euo pipefail) to ensure it fails fast and properly handles errors.
63-64: Good log message on completionThe final success message provides clear feedback that the process has completed successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/destroy.yml (1)
52-58: Environment Name Consistency in Lambda Deletion Step
In the "Delete Lambda@Edge function" step, the script is invoked with${{ vars.ENV_NAME }}rather than using the transformed value from the previous step (e.g.${{ steps.get_env_name.outputs.lowercase }}). Please verify that the./deployment/delete_lambdascript accepts the raw environment name. If a specific format is expected, consider updating the argument accordingly to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/destroy.yml(1 hunks)doc/release/RELEASE-NOTES.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release/RELEASE-NOTES.md
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-django-code-quality
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-countries-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-django-cov
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployment/delete_lambda (1)
15-26:⚠️ Potential issueInitialize DISTRIBUTION_ID before first use.
The variable
DISTRIBUTION_IDis checked at line 15 but never initialized. This could cause unexpected behavior due to the-uoption which treats unset variables as errors.ENV_NAME=$1 CLOUDFRONT_DOMAIN=$2 +DISTRIBUTION_ID="" LAMBDA_ARN="arn:aws:lambda:$REGION:$ACCOUNT_ID:function:funcOpenSupplyHub${ENV_NAME}RedirectToS3origin"
🧹 Nitpick comments (5)
deployment/delete_lambda (5)
24-26: Fix indentation in error message.The error message for missing CloudFront distribution should be properly indented to maintain consistent code style.
if [ -z "$DISTRIBUTION_ID" ]; then -echo "Error: No CloudFront distribution found for domain: $CLOUDFRONT_DOMAIN" -exit 1 + echo "Error: No CloudFront distribution found for domain: $CLOUDFRONT_DOMAIN" + exit 1 fi
33-33: Consider handling Lambda associations in all cache behaviors.The script only removes Lambda function associations from the default cache behavior. If the function is associated with other cache behaviors, it won't be detached.
-CONFIG=$(echo "$CONFIG" | jq 'del(.DefaultCacheBehavior.LambdaFunctionAssociations.Items[] | select(.LambdaFunctionARN == "'$LAMBDA_ARN':'$LAMBDA_VERSION'"))') +# Remove Lambda from default cache behavior +CONFIG=$(echo "$CONFIG" | jq 'del(.DefaultCacheBehavior.LambdaFunctionAssociations.Items[] | select(.LambdaFunctionARN == "'$LAMBDA_ARN':'$LAMBDA_VERSION'"))') + +# Remove Lambda from all cache behaviors if present +CONFIG=$(echo "$CONFIG" | jq 'if has("CacheBehaviors") then .CacheBehaviors.Items = (.CacheBehaviors.Items | map(del(.LambdaFunctionAssociations.Items[] | select(.LambdaFunctionARN == "'$LAMBDA_ARN':'$LAMBDA_VERSION'")))) else . end')
1-8: Add validation for required arguments.The script accepts two arguments but doesn't validate that they are provided. If either argument is missing, the script will fail with a cryptic error.
#!/bin/bash set -euo pipefail +# Validate required arguments +if [ $# -lt 2 ]; then + echo "Usage: $0 <environment_name> <cloudfront_domain>" + exit 1 +fi + ACCOUNT_ID=$(aws sts get-caller-identity --query Account --output text) REGION="us-east-1" ENV_NAME=$1 CLOUDFRONT_DOMAIN=$2
41-44: Add error handling for CloudFront update operation.The script doesn't handle potential errors from the CloudFront update operation. If the update fails, the script will continue to delete the Lambda function, which might not be desirable.
echo "Waiting for CloudFront distribution deployment..." -aws cloudfront wait distribution-deployed --id "$DISTRIBUTION_ID" +if ! aws cloudfront wait distribution-deployed --id "$DISTRIBUTION_ID"; then + echo "Error: CloudFront distribution deployment failed. Aborting Lambda function deletion." + exit 1 +fi echo "CloudFront distribution updated. Proceeding with Lambda function deletion..."
12-14: Improve domain matching logic for CloudFront distributions.The current exact string comparison may not work if a distribution has multiple domains, as
domainswould be a space-separated list.for id in $(aws cloudfront list-distributions --query "DistributionList.Items[*].Id" --output text); do - domains=$(aws cloudfront get-distribution-config --id $id --query "DistributionConfig.Aliases.Items" --output text) - if [[ "$domains" == "$CLOUDFRONT_DOMAIN" ]]; then + domains=$(aws cloudfront get-distribution-config --id "$id" --query "DistributionConfig.Aliases.Items" --output text) + if echo "$domains" | grep -q "\b$CLOUDFRONT_DOMAIN\b"; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/destroy.yml(1 hunks)deployment/delete_lambda(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/destroy.yml
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (2)
deployment/delete_lambda (2)
1-11: LGTM: Script properly implements Lambda@Edge deletion.The script correctly sets up error handling, retrieves the AWS account ID dynamically, and prepares the Lambda function ARN based on the environment name. It also properly determines the latest Lambda version.
12-13: Quote the CloudFront distribution ID variable.When accessing a CloudFront distribution, the ID should be quoted to handle cases where it might contain special characters.
for id in $(aws cloudfront list-distributions --query "DistributionList.Items[*].Id" --output text); do - domains=$(aws cloudfront get-distribution-config --id $id --query "DistributionConfig.Aliases.Items" --output text) + domains=$(aws cloudfront get-distribution-config --id "$id" --query "DistributionConfig.Aliases.Items" --output text)
3 participants
Delete Lambda@Edge function before deleting infrastructure. These changes are necessary so that infrastructure can be deleted using terraform without receiving an error about a problem deleting a replicated function.