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

Improve SQL User Lambda Delete operation #54464

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Improve SQL User Lambda Delete operation #54464

merged 3 commits into from
Oct 25, 2023

Conversation

sureshc
Copy link
Contributor

@sureshc sureshc commented Oct 24, 2023 •
edited

During end-to-end testing of the SQL User lambda in #52796 we found that when a CloudFormation template incorrectly allows for a database resource to be deleted before a SQLUser resource, the delete operation fails on the SQLUser resource even though deleting the database effectively deletes the user.

Testing story

JS tests

sql-user % npm test

> sql-user@1.0.0 test
> mocha *.test.js



  SQL User Custom Resource
REQUEST RECEIVED:
 {"RequestType":"Create","ResourceProperties":{"Privileges":["ALL PRIVILEGES"]},"PhysicalResourceId":"someId"}
CREATE USER SQL Statement Execution Results:
 {}
UPDATE USER SQL Statement Execution Results:
 {}
GRANT USER SQL Statement Execution Results:
 [ {}, {} ]
    ✔ should handle Create event successfully
REQUEST RECEIVED:
 {"RequestType":"Delete","ResourceProperties":{},"PhysicalResourceId":"someId"}
DELETE USER SQL Statement Execution Results:
 {}
    ✔ should handle Delete event successfully


  2 passing (23ms)

CloudFormation validate

% bundle exec rake stack:lambda:validate
Finished stack:lambda:environment (less than a minute)
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn  --production
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig npm install  ci --only=prod
Uploading Lambda zip package to S3 (14455998 bytes)...
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
Pending update for stack `lambda`:
Modify SQLUserLambda [AWS::Lambda::Function] Properties (Code)
Finished stack:lambda:validate (1 minute)

Deployment strategy

$ bundle exec rake stack:lambda:start

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@sureshc sureshc requested a review from a team as a code owner October 24, 2023 23:07
cat5inthecradle
cat5inthecradle approved these changes Oct 25, 2023
View reviewed changes
aws/cloudformation/lambdas/sql-user/index.js Outdated
console.log(results.createUser); // Results from the CREATE USER query.
console.log(results.updateUser); // Results from the UPDATE USER query.
console.log(results.grantResults); // Array of results from each GRANT operation.
// Results from the CREATE USER query.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comments are no longer necessary with these logging messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

aws/cloudformation/lambdas/sql-user/index.js Outdated Show resolved Hide resolved
aws/cloudformation/lambdas/sql-user/index.js Outdated
return await queryPromise(connection, dropUser);
return await queryPromise(connection, dropUser).catch((error) => {
if (error.code === "ENOTFOUND") {
let databaseDeletedAlreadyMessage = `
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

sureshc and others added 2 commits October 24, 2023 22:41
@sureshc sureshc merged commit aff32a0 into staging Oct 25, 2023
1 of 2 checks passed
@sureshc sureshc deleted the fix-sql-user-lambda branch October 25, 2023 05:52
@sureshc
Copy link
Contributor Author

sureshc commented Oct 25, 2023

Deployed

code-dot-org % bundle exec rake stack:lambda:start
Finished stack:lambda:environment (less than a minute)
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn  --production
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig npm install  ci --only=prod
PKG_CONFIG_PATH=/usr/X11/lib/pkgconfig yarn
Pending update for stack `lambda`:
Modify SQLUserLambda [AWS::Lambda::Function] Properties (Code)
Proceed? [y/n]
y
 Stack update requested, waiting for provisioning to complete...
.2023-10-25 05:49:19 UTC- SQLUserLambda [UPDATE_IN_PROGRESS]
..2023-10-25 05:49:26 UTC- SQLUserLambda [UPDATE_COMPLETE]
2023-10-25 05:49:28 UTC- lambda [UPDATE_COMPLETE_CLEANUP_IN_PROGRESS]

Stack update complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cat5inthecradle cat5inthecradle cat5inthecradle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sureshc @cat5inthecradle