Skip to content

Conversation

@davidcheung
Copy link
Contributor

@davidcheung davidcheung commented Jul 6, 2020

The backend service expects a sercet with DB_PASSWORD and DB_USERNAME
Currently we provision an RDS with a master password, and its a horrible idea for application to use master password to connect to db, this PR creates an extra step during apply to create a application specific user in the db and a k8s-secret

  • the manifest creates 4 things
    • Namespace: db-ops
    • Secret in db-ops: db-create-users (with master password, and a .sql file
    • Job in db-ops: db-create-users (runs the .sql file against the RDS given master_password from env)
    • Secret in Application namespace with DB_USERNAME / DB_PASSWORD
  • then it deletes the db-ops namespace leaving only the Secret in Application namespace behind

# docker image with postgres client only
DOCKER_IMAGE_TAG=governmentpaas/psql:latest

DB_ENDPOINT=$(aws rds describe-db-instances --region=$REGION --query "DBInstances[?DBName=='$PROJECT_NAME'].Endpoint.Address" | jq '.[0]' | sed "s/\"//g")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative we may want to explore here which would make this easier to use is in the aws-eks repo we could create an "externalname" service in the cluster which points to the database, then we could refer to it by a static name rather than having to get the RDS hostname.

Also if you use jq -r you don't need that sed at the end.

Copy link
Contributor

@bmonkman bmonkman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Requested a couple changes.


# Deleting the entire db-ops namespace, leaving ONLY application-namespace's secret behind
kubectl wait --for=condition=complete --timeout=10s job db-create-users
kubectl delete namespace db-ops
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intent is for the previous command to kill the script if it fails you should use set -e at the top but it's probably a better ideal to check if the return value of the previous command is zero and then delete the namespace. We probably want to keep the namespace if the job failed to debug.

Copy link
Contributor Author

@davidcheung davidcheung Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somelike like?

Suggested change
kubectl delete namespace db-ops
EXIT_CODE=$?
if [ $EXIT_CODE -eq 0 ]
then
kubectl delete namespace db-ops
if

Copy link
Contributor

@bmonkman bmonkman Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! (Though needs syntax fix, and you could just have $? in the comparison without assigning it.)
And you could have an else in there with a nicer message like "Failed to create an application user in the database - you can find out more by running kubectl -n db-ops get logs blahblah"

then
kubectl delete namespace db-ops
else
POD_NAME=$(kubectl -n db-ops get po | grep db-create-users |awk '{print $1}')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could clean this up a bit by giving the pod spec in the job a label like app: db-create-users and then a user could run kubectl logs -n db-ops -l app=db-create-users

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice, didnt know it supports labels 👁️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like they come with a job-name label, gonna use that instead
image

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.

3 participants