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

Add Grant support to be defined in MySQLUser #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bogdankatishev
Copy link

Hello,

this PR adds grant support inside Custom::MySQLUser and 3 additional properties:

- `Grant` - the privileges to grant
- `GrantOn` - the privilege level to grant, use . for global grants. Update requires replacement.
- `WithGrantOption` - if the user is allowed to grant others, defaults to false

As seen from the demo-stack.yaml:

      Grant:
      - 'SELECT'
      GrantOn: 'root.*'
      WithGrantOption: false

@mvanholsteijn
Copy link
Member

Thank you for the PR.

my CFN philosophy is that each resource with a independent lifecycle, should be implemented as a separate Resource.

This MR combines the lifecycle of the user with that of a grant. May be I am mistaken, but I believe that when you change the grants of a user, it will first update and return a modified physical resource id. AFAIK, CFN is going to issue a delete after the update using the old physical resource id. 🤔

There is a MR outstanding for #8 which implements it this way. As that MR was pretty huge, I did not get around to pick it apart.

Let me get back to you.

@bogdankatishev
Copy link
Author

Hello,

The reason I added these properties directly to MySQLUser was because: I saw a lot of duplicate code being used. One was for creating the MySQLUser and the other one was for creating the grant. When comparing them, I saw a lot of same code being used.

I also find that the grant is tightly coupled to the mysql-user. (but that is my own opinion)

I also fixed the bug that you mentioned above in commit: 9c5da74

Now the recreation of the resource will only be triggered if one the following properties changes:

  • User
  • DBName

@mvanholsteijn
Copy link
Member

Thank you for fixing the issue. I had just one more thought about the grants being tied to the user. Normally, I create the user in CloudFormation so that I can generate a random password and hand the credentials over to the application via de parameter store. The application teams are responsible for managing the database schema and database grants.

This may cause conflict with the CFN managed grants: permissions granted by the application may be revoked on update of CFN.

What is your strategy for dealing with this?

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.

2 participants