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.

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

feat: support multiple connection Uris for SQL databases #2481

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

phani-srikar
Copy link
Contributor

@phani-srikar phani-srikar commented Apr 19, 2024

Description of changes

With this feature, we will allow the customers of Amplify GraphQL CDK construct to specify more than one SSM paths that hold the connection URI for the SQL database. This enables customers of Amplify Console to have both shared and branch specific secrets for database connection URIs.
The provided SSM paths are resolved in the order that is specified in the array by the customer and based on the availability to read them at runtime during execution of the SQL Lambda datasource.
This PR also updates the error messages to make them more readable.
The change is backwards compatible since existing customers can continue to specify the connection URI SSM path as a single string.

CDK / CloudFormation Parameters Changed

Issue #, if available

Description of how you validated changes

Added unit and E2E tests. The added E2E test runs successfully on CI.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
  • Any CDK or CloudFormation parameter changes are called out explicitly

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@phani-srikar phani-srikar requested review from a team as code owners April 19, 2024 21:11
palpatim
palpatim previously approved these changes Apr 19, 2024
Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

Approved contingent on @atierian 's approval of public API change.

atierian
atierian previously approved these changes Apr 19, 2024
return await getSSMValue(parsedJsonConnectionString);
}
catch (e) {
console.log(ssmLoggedError);
Copy link
Contributor

Choose a reason for hiding this comment

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

These logs may be redundant. IIRC, exceptions are recorded in CW logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These ones are intentional since the logged message has more details than the error message we send back to the GraphQL client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did a quick testing, these log statements are redundant. Throwing an error automatically records the full error object as shown in the screenshot.

  const customErr = 'This is a custom error';
  console.log(customErr);
  throw new Error(customErr);
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is expected. The situation here is different from above case since the message we log is different from the error we throw which also gets logged but has less information than the additionally added log.

const ssmRequestError = 'Unable to connect to the database. Check the logs for more details.';
const ssmLoggedError = 'Unable to fetch the connection Uri from SSM for the provided paths.';

console.log(ssmLoggedError);
throw new Error(ssmRequestError);

So the log message is useful since it additionally tells the developer that: We failed to connect to DB because we couldn't fetch the connection Uri from SSM.
The reason this isn't put behind DEBUG_FLAG setting is because this is a custom error and does not print out any sensitive information other than what we already expect from our public docs and saves the developer time to debug the issue without needing to update the environment variable to debug.

@phani-srikar phani-srikar dismissed stale reviews from atierian and palpatim via ca6a34f April 22, 2024 17:21
@phani-srikar phani-srikar merged commit 7ea8000 into main Apr 23, 2024
7 checks passed
@phani-srikar phani-srikar deleted the add-multiple-connection-uri-paths branch April 23, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants