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

Feature/add relational database datasource #226

Merged
merged 5 commits into from Mar 16, 2019

Conversation

Projects
None yet
3 participants
@roznalex
Copy link
Contributor

roznalex commented Mar 12, 2019

Hi, @sid88in. I've added support for this feature. Could you accept the pull request?

Show resolved Hide resolved index.test.js Outdated
Show resolved Hide resolved index.test.js Outdated
Show resolved Hide resolved index.test.js Outdated
Show resolved Hide resolved index.test.js Outdated
Show resolved Hide resolved index.test.js Outdated
Show resolved Hide resolved index.test.js Outdated
@sid88in

This comment has been minimized.

Copy link
Owner

sid88in commented Mar 14, 2019

@roznalex thanks a lot for contributing to this feature. this is amazing. can you please do the following before we merge this:

  1. remove your hardcoded roles/creds and make this code generic.
  2. add details in the readme on how to use this feature (including change in YAML)
  3. are there any vpc requirements which needs to be set for RDS integ with AppSync
  4. add screenshots in this PR showing E2E run
@bboure
Copy link
Collaborator

bboure left a comment

@roznalex THanks for the effort! 🎉
Code LGTM, but I need to have a look at AWS Doc to understand how RDS data sources work first.

Show resolved Hide resolved index.test.js Outdated
@bboure
Copy link
Collaborator

bboure left a comment

Please review my latest comments.

index.js Outdated
secretResourceArn,
{ "Fn::Join" : [ ":", [dbResourceArn, '*'] ] },
{ "Fn::Join" : [ ":", [secretResourceArn, '*'] ] },
];

This comment has been minimized.

@bboure

bboure Mar 14, 2019

Collaborator

@roznalex I think something is wrong here.
Look at the doc example: https://docs.aws.amazon.com/appsync/latest/devguide/tutorial-rds-resolvers.html

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "rds-data:DeleteItems",
                "rds-data:ExecuteSql",
                "rds-data:GetItems",
                "rds-data:InsertItems",
                "rds-data:UpdateItems"
            ],
            "Resource": [
                "arn:aws:rds:us-east-1:123456789012:cluster:mydbcluster",
                "arn:aws:rds:us-east-1:123456789012:cluster:mydbcluster:*"
            ]
        },
        {
            "Effect": "Allow",
            "Action": [
                "secretsmanager:GetSecretValue"
            ],
            "Resource": [
                "arn:aws:secretsmanager:us-east-1:123456789012:secret:mysecret",
                "arn:aws:secretsmanager:us-east-1:123456789012:secret:mysecret:*"
            ]
        }
    ]
}

We need 2 statements in the policy, one for accessing the RDS cluster, and one for accessing the secret manager.
But here it looks like you are generating one single statement that can access both resources.
On top of that, I am missing the secretsmanager:GetSecretValue action for the secret SecretsManager.

I think we need some small refactoring of getDefaultDataSourcePolicyStatements here.
It should return an array of statements, instead of one single statement.
For all the pre-existing data sources, nothing changes and it returns an array of one item.
For RDS, it will return 2 statements, one for the RDS endpoint, and another for SecretsManager.

This comment has been minimized.

@roznalex

roznalex Mar 15, 2019

Author Contributor

Hi, @bboure. Just updated the PR. Could you check it, please?

DbClusterIdentifier: ds.config.dbClusterIdentifier,
DatabaseName: ds.config.databaseName,
Schema: ds.config.schema,
AwsSecretStoreArn: ds.config.awsSecretStoreArn,

This comment has been minimized.

@bboure

bboure Mar 14, 2019

Collaborator

@roznalex would it make sense to also auto-generate the secret store for the datasource?

awsSecretStoreArn: arn:...... # used if provided
secretSotreConfig: #used if awsSecretStoreArn is not provided
  username: USERNAME 
  password: COMPLEX_PASSWORD

If awsSecretStoreArn is provided, it would be used, but if not and secretSotreConfig is present, the plugin would do the following:

  • auto-generate a AWS::SecretsManager::Secret resource
  • store the given username and password in it
  • resolve its ARN and inject it into awsSecretStoreArn

Not sure if it makes sense.
How do you currently handle secrets? Do you generate the AWS::SecretsManager::Secret in serverless ?
The tricky part here is handling the secrets without leaking them in the yml file.

This comment has been minimized.

@roznalex

roznalex Mar 14, 2019

Author Contributor

I create the "AWS::SecretsManager::Secret" resource in a Cloudformation template where I pass all the secrets through environment variables.

    Type: "AWS::SecretsManager::Secret"
    Properties:
      Name: "rds-cluster-secret-name"
      Description: RDS cluster credentials
      SecretString: '{"username":"${self:custom.environment.RDS_USERNAME}","password":"${self:custom.environment.RDS_PASSWORD}"}'

I am not sure about the autogeneration that you mentioned above. But we might think about it.

This comment has been minimized.

@bboure

bboure Mar 14, 2019

Collaborator

@roznalex 👍
it was just an idea... it could also come in a future iteration.
We could start with a simple implementation

@roznalex

This comment has been minimized.

Copy link
Contributor Author

roznalex commented Mar 14, 2019

@sid88in @bboure thank you for the comments. I will try to fix all of them soon.

@bboure
Copy link
Collaborator

bboure left a comment

Thank you @roznalex
Please see my latest comments.
Apart from that, LGTM 👍 🎢

README.md Outdated
region: # Overwrite default region for this data source
- type: RELATIONAL_DATABASE
name: # data source name
description: # DynamoDB Table Description

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex Relational Database Description ?

awsSecretStoreArn: { Ref: RDSClusterSecret } # Where RDSClusterSecret is the RDS cluster secret defined in Resources
serviceRoleArn: { Fn::GetAtt: [RelationalDbServiceRole, Arn] } # Where RelationalDbServiceRole is an IAM role defined in Resources
databaseName: # optional database name
schema: # optional database schema

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex I believe this field has a closed list of possible values? If so, can we list them here?

This comment has been minimized.

@roznalex

roznalex Mar 15, 2019

Author Contributor

@bboure according to the documentation, there is no list of possible values for the "schema" field.

index.js Outdated
],
Resource: [
ds.config.dbClusterIdentifier,
`${ds.config.dbClusterIdentifier}:*`,

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex Is this going to work if ds.config.dbClusterIdentifier contains a reference like in your example? { Fn::GetAtt: [DBCluster, Arn] }
Don't we need to use Fn:Join like in Lambda and DynamoDb above ?

This comment has been minimized.

@roznalex

roznalex Mar 15, 2019

Author Contributor

@bboure you are right, we can use Fn:Join here.

This comment has been minimized.

@roznalex

roznalex Mar 16, 2019

Author Contributor

@bboure, done.

index.js Outdated
],
Resource: [
ds.config.awsSecretStoreArn,
`${ds.config.awsSecretStoreArn}:*`,

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex same here

This comment has been minimized.

@roznalex

roznalex Mar 15, 2019

Author Contributor

@bboure we should use the full "awsSecretStoreArn" here because this ARN is generated dynamically and cannot be preconfigured.

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex I'm not sure I understand.
We cannot use Ref or Fn::GetAttr to get the ARN here?

This comment has been minimized.

@roznalex

roznalex Mar 15, 2019

Author Contributor

@bboure I use Ref within my project. It returns ARN. Please, see the Ref section here

This comment has been minimized.

@bboure

bboure Mar 15, 2019

Collaborator

@roznalex yes. So, I believe we need to use Join here too then.
Otherwise, here we would be concatenating an object with text.

@bboure

bboure approved these changes Mar 16, 2019

Copy link
Collaborator

bboure left a comment

🎉 Thanks @roznalex LGTM
Will merge this today. unless you want to add something?

@bboure

bboure approved these changes Mar 16, 2019

Copy link
Collaborator

bboure left a comment

🎉 Thanks @roznalex LGTM
Will merge this today. unless you want to add something?

@roznalex

This comment has been minimized.

Copy link
Contributor Author

roznalex commented Mar 16, 2019

🎉 Thanks @roznalex LGTM
Will merge this today. unless you want to add something?

I believe that's all. Thank you!

@bboure bboure merged commit c04273c into sid88in:master Mar 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.