-
Notifications
You must be signed in to change notification settings - Fork 207
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
Database Resource Provider #683
Conversation
@5herlocked would love to see this extend to RDS as an addon I'm working on (Apache Airflow) needs RDS for DB. |
Will start working on it this week |
As requested, RDS Instance provider has been built. Will be adding in test cases promptly. |
@youngjeong46 Looks like I need to update the RP for the newest version of the VPC RP. |
@shapirov103 @youngjeong46 Please leave feedback if considered required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5herlocked Why do we need this? Could we not use ACK or CrossPlane to create RDS instance? Want to understand the motivation. Also please make sure you add an example of this resource provider in example stack and also update the documentation for using this resource-provider
Absolutely, we can use crossplane or ACK. Though it would require a fair bit of setup to ensure the RDS instance/cluster's lifecycle would match that of the EKS cluster. Right now if I understand it right, the process to setup an RDS cluster/instance supporting an EKS cluster is one of the following:
With this, we can bypass ACK and directly attach the Lifecycle of a Blueprinted cluster to an RDS instance supporting to make the flow:
I believe this more closely matches with the spirit of the repository than using ACK, as this considers the RDS instance as a part of the infrastructure required by the cluster and provides the ability to create and delete it as part of the infra pipeline. Will add the examples and create the documentation before EoW. |
@5herlocked First of all, thank-you for the PR and also response. Having RDS and managing its lifecycle strictly as part of Cluster creation is strictly a No-No. Lifecycle of RDS should be ideally maintained outside of the lifecycle of the EKS cluster creation so even if the cluster is deleted, RDS remains and data persisted in RDS remains. For S3 its a different usecase where you might need to create a bucket for temporary purposes like Airflow usecase. But in case of RDS its a strict anti pattern to manage the RDS creation with EKS Cluster creation. The reason why i suggested to use ACK or CrossPlane is for multi cluster management scenarios where you have a management cluster which creates child cluster and RDS databases. So bottom line is |
@elamaran11 and myself discussed it. It should be made very clear in the documentation that this addon is coupled to the lifecycle of the cluster (which is the case for any resource tbh). It can only be used with a so-called management cluster that governs the platform. |
@shapirov103 @elamaran11 Will it make sense to, by default set the retention policy to snapshot when deleting as a way of safeguarding customer data? |
@5herlocked let's resolve the conflicts and I agree with the direction of applying a snapshot or any more conservative retention policy to avoid inadvertent data loss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comment on the issue on retention and docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5herlocked great work! a few minor comments on the doc
@5herlocked Did you get a chance to close this PR comments. |
Yes, it was as part of the last commit. Please feel free to look it over. |
This PR has been automatically marked as stale because it has been open 60 days |
@elamaran11 feel free to close/merge. It is complete and the tests are inplace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @shapirov103 Please check from your end.
Hey @5herlocked how would this work in terms of network connectivity? I don't any security group defined or imported here. |
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests failed. A maintainer can provide more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5herlocked have you validated functionally on a cluster? I see this is not included in the e2e test as it stands. If you can attach the blueprint you used to validate and add a screenshot of a command (kubectl or aws cli call) that validates it wokrs, that would help merge it.
Please also see the question wrt to network connectivity. How do you envision that part - should there be an extension for SG configuration? we can defer it, but it seems like a common requirement from the customers
@sanyer Networking configuration is entirely inherited from the VPC Resource Provider. Additional configurations are can also be transparently passed to the underlying resources, note the interface @shapirov103 Will include deployment screenshots and necessary e2e tests ASAP. |
@5herlocked whenever you get a chance, lets address the above and merge it. The functionality looks really good, I assume there will be more requests around this capability once customers start using it. |
This PR has been automatically marked as stale because it has been open 60 days |
… integrate it into PostgresProps.auth.existingSecret
… provider. Updated both RPs to also take in arbitrary props for both while enforcing EKS best practices (all the vpc stuff)
adcb47f
to
8d169df
Compare
@5herlocked please merge main to avoid MD link failure |
Underlying Database Cluster CDK constructs changed. This is no longer functional. --- Reverting to draft to work on it --- |
This PR has been automatically marked as stale because it has been open 60 days |
Closing for now, we can reopen if needed. |
Issue #, if available: In support of #456; #651
Description of changes: Addition of an extensible RDS resource provider - currently only supports Aurora, but will be adding additional support in the coming months as demand changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.