-
Notifications
You must be signed in to change notification settings - Fork 59
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
Introduce SQLClaim that can handle any RDS database engine #88
Conversation
* PostgrSQLInstance -> SQLInstance * expose `engine` and `engineVersion` in XRD * Add 3306 SecurityGroupRule * Provide mariadb example Signed-off-by: Yury Tsarev <yury@upbound.io>
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.
Approved with a few minor comments.
kubectl get -f postgres-claim.yaml
NAME SYNCED READY CONNECTION-SECRET AGE
sqlinstance.aws.platformref.upbound.io/platform-ref-aws-db-postgres True True platform-ref-aws-db-conn-postgres 31m
NAME TYPE DATA AGE
secret/psqlsecret Opaque 1 31m
kubectl get -f mariadb-claim.yaml
NAME SYNCED READY CONNECTION-SECRET AGE
sqlinstance.aws.platformref.upbound.io/platform-ref-aws-db-mariadb True True platform-ref-aws-db-conn-mariadb 4m10s
- directory
database/postgres
be something likedatabase/sql
- The readme should indicate that the cluster is required to provision the DBs. In the graph in the readme they look like independent paths.
examples/mariadb-claim.yaml
Outdated
password: dXBiMHVuZHIwY2s1ITMxMzM3 | ||
kind: Secret | ||
metadata: | ||
name: psqlsecret |
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.
should this be mariadbsecret
?
examples/mariadb-claim.yaml
Outdated
storageGB: 5 | ||
passwordSecretRef: | ||
namespace: default | ||
name: psqlsecret |
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.
mariadbsecret
?
@@ -29,6 +29,12 @@ spec: | |||
parameters: | |||
type: object | |||
properties: | |||
engine: |
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.
Should this be an enum
of supported db types?
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.
I was thinking about it but do not want to immediately maintain everything that is described in https://docs.aws.amazon.com/AmazonRDS/latest/APIReference/API_CreateDBInstance.html
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.
I'd just support mariadb and postgres in the enum.
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.
that make sense, I will amend the PR
* Add enum for engine type validation * Rename mariadb claim secret * Rename db abstraction directory * Use `dbName` to create custome database within the instance by default so it can be consumed by aribtrary app Signed-off-by: Yury Tsarev <yury@upbound.io>
@stevendborrelli PR amended, could you please take another brief look? |
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, cluster + mariadb provisioned successfully.
Description of your changes
Instead of hardcoded PostgreSQL, make generic SQL XRD/Composition that can handle arbitrary RDS instance engine configuration
engine
andengineVersion
in XRDSigned-off-by: Yury Tsarev yury@upbound.io
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
PostgreSQL connectivity test from within instantiated EKS cluster
mariadb connectivity test from within instantiated EKS cluster