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

Database Resource Provider #683

Closed
wants to merge 12 commits into from

Conversation

5herlocked
Copy link
Contributor

@5herlocked 5herlocked commented May 12, 2023

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.

@youngjeong46
Copy link
Collaborator

@5herlocked would love to see this extend to RDS as an addon I'm working on (Apache Airflow) needs RDS for DB.

@5herlocked 5herlocked changed the title Aurora Resource Provider Database Resource Provider May 23, 2023
@5herlocked
Copy link
Contributor Author

Will start working on it this week

@5herlocked 5herlocked marked this pull request as draft May 23, 2023 19:22
@5herlocked 5herlocked marked this pull request as ready for review May 24, 2023 18:30
@5herlocked
Copy link
Contributor Author

As requested, RDS Instance provider has been built.

Will be adding in test cases promptly.

@5herlocked
Copy link
Contributor Author

5herlocked commented May 24, 2023

@youngjeong46 Give this a try.
It does successfully create an RDS and provides a secret as an ISecret.
Should be pretty straight forward to integrate it into a full deployment.

Looks like I need to update the RP for the newest version of the VPC RP.

@5herlocked
Copy link
Contributor Author

@shapirov103 @youngjeong46 Please leave feedback if considered required.
Will fillout additional documentation towards EoW.

Copy link
Collaborator

@elamaran11 elamaran11 left a 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

@5herlocked
Copy link
Contributor Author

5herlocked commented May 25, 2023

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:

  • EKS Blueprints -> ACK -> Argo Managed RDS YAMLs
  • EKS Blueprints -> ACK -> Pre-Existing RDS YAMLs (probably still managed through argo)

With this, we can bypass ACK and directly attach the Lifecycle of a Blueprinted cluster to an RDS instance supporting to make the flow:

  • EKS Blueprints -> RDS with the same configurations that you get with the CDK resources DatabaseInstanceand DatabaseCluster

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.

@elamaran11
Copy link
Collaborator

elamaran11 commented May 25, 2023

@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 RDSProvider does not align to the spirit of ResourceProvider so i would question on why we needs this, what is your motivation and usecase if the answer is RDS creation with cluster, i wouldn't necessarily agree. Using ACK or CrossPlane with Argo or Flux promotes GitOps which would be a right approach in your case.

@shapirov103
Copy link
Collaborator

@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.
It can also be used for illustrative purposes, however it makes sense to check resource retention control to avoid accidental data loss when stack is reprovisioned or dropped.
In the management cluster pattern, this resource provider is not that different from any resources provisioned with ACK or Crossplane. Removal of the resource from the management cluster by default will drop such resources as well.
I can see Ela's perspective as more generalizing - e.g. what if we now add a bunch of different database providers and then customers may misuse it. We need to define an overall strategy for resource providers with the goal of addressing real customer use cases and improving customer experience the best we can.

@5herlocked
Copy link
Contributor Author

@shapirov103 @elamaran11 Will it make sense to, by default set the retention policy to snapshot when deleting as a way of safeguarding customer data?

@shapirov103
Copy link
Collaborator

@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.
We will need a doc under docs/resource-providers (index.md and the rds.md).

Copy link
Collaborator

@shapirov103 shapirov103 left a 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.

Copy link
Collaborator

@shapirov103 shapirov103 left a 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

docs/resource-providers/rds-providers.md Outdated Show resolved Hide resolved
docs/resource-providers/rds-providers.md Outdated Show resolved Hide resolved
docs/resource-providers/rds-providers.md Outdated Show resolved Hide resolved
@elamaran11
Copy link
Collaborator

@5herlocked Did you get a chance to close this PR comments.

@5herlocked
Copy link
Contributor Author

Yes, it was as part of the last commit. Please feel free to look it over.

Copy link

github-actions bot commented Dec 8, 2023

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Dec 8, 2023
@elamaran11 elamaran11 removed the stale label Dec 8, 2023
@5herlocked
Copy link
Contributor Author

@elamaran11 feel free to close/merge. It is complete and the tests are inplace.

Copy link
Collaborator

@elamaran11 elamaran11 left a 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.

@sanyer
Copy link

sanyer commented Feb 3, 2024

Hey @5herlocked how would this work in terms of network connectivity? I don't any security group defined or imported here.

@shapirov103
Copy link
Collaborator

/do-e2e-tests

Copy link

@aws-ia-ci aws-ia-ci left a 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.

Copy link
Collaborator

@shapirov103 shapirov103 left a 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

@5herlocked
Copy link
Contributor Author

5herlocked commented Feb 19, 2024

@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 CreateRDSInstanceProviderProps.rdsProps. For additional reference: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds.InstanceProps.html AND https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds.DatabaseClusterProps.html

@shapirov103 Will include deployment screenshots and necessary e2e tests ASAP.

@shapirov103
Copy link
Collaborator

@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.

Copy link

github-actions bot commented Jun 3, 2024

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@5herlocked 5herlocked force-pushed the aurora-rp branch 2 times, most recently from adcb47f to 8d169df Compare June 21, 2024 18:36
@shapirov103
Copy link
Collaborator

@5herlocked please merge main to avoid MD link failure

@5herlocked
Copy link
Contributor Author

5herlocked commented Jun 26, 2024

Underlying Database Cluster CDK constructs changed. This is no longer functional.

--- Reverting to draft to work on it ---

@5herlocked 5herlocked marked this pull request as draft June 26, 2024 03:26
Copy link

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Sep 25, 2024
@shapirov103
Copy link
Collaborator

Closing for now, we can reopen if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants