Skip to content
This repository has been archived by the owner on Dec 21, 2021. It is now read-only.

[DDO-1292] Rawls Liquibase Migration Pre-sync Job #387

Merged
merged 37 commits into from
Jun 15, 2021
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
39db818
No-op migration test capability
jack-r-warren Jun 7, 2021
92a77c8
newlines
jack-r-warren Jun 7, 2021
1af9322
bump minor
jack-r-warren Jun 7, 2021
0d4848d
update boilerplate
jack-r-warren Jun 7, 2021
f581aee
nindent
jack-r-warren Jun 7, 2021
cd23d17
resources in earlier wave
jack-r-warren Jun 7, 2021
808c616
shareProcessNamespace
jack-r-warren Jun 8, 2021
d25b2fa
Add cloudsql sleep
jack-r-warren Jun 8, 2021
46c3e64
Sleep for 15s
jack-r-warren Jun 8, 2021
26a39b8
fix hostname
jack-r-warren Jun 8, 2021
23f17db
liquibase kill cloudsql proxy
jack-r-warren Jun 8, 2021
76c2d67
Exit instead of return
jack-r-warren Jun 8, 2021
7fa6fbc
Don't retry so many times
jack-r-warren Jun 8, 2021
0c4cd87
the other spec
jack-r-warren Jun 8, 2021
7154600
Control no-op and failure behavior via values
jack-r-warren Jun 9, 2021
a9f6786
Bring in the new liquibase.properties
jack-r-warren Jun 10, 2021
578b5ad
Merge branch 'master' of github.com:broadinstitute/terra-helm into DD…
jack-r-warren Jun 10, 2021
67ad053
Merge branch 'master' of github.com:broadinstitute/terra-helm into DD…
jack-r-warren Jun 10, 2021
93c6348
clean up
jack-r-warren Jun 10, 2021
2bc9e75
line continuation
jack-r-warren Jun 14, 2021
8d086c6
Use official google image
jack-r-warren Jun 14, 2021
48407c5
Sleep forever so I can see the pod
jack-r-warren Jun 14, 2021
d3f562f
Fix version, normal sleep
jack-r-warren Jun 14, 2021
d9a1b08
No exec
jack-r-warren Jun 14, 2021
1baf129
alpine for sh
jack-r-warren Jun 14, 2021
c0b105f
labels
jack-r-warren Jun 14, 2021
77588ea
delete on succeeded
jack-r-warren Jun 14, 2021
12a09f8
Different syntax
jack-r-warren Jun 14, 2021
fc61125
No extra deletion policy
jack-r-warren Jun 14, 2021
c96ba52
prefix with rawls
jack-r-warren Jun 14, 2021
b4b586b
New readme
jack-r-warren Jun 14, 2021
4348a39
Put rbac behind flag, disable by default
jack-r-warren Jun 15, 2021
7d48165
spell the deletion policy field correctly :(
jack-r-warren Jun 15, 2021
e39fd66
enable rbac
jack-r-warren Jun 15, 2021
7c85941
reuse rbac, move it to earlier wave
jack-r-warren Jun 15, 2021
abe0064
quotes
jack-r-warren Jun 15, 2021
fa53069
secretdef in same wave
jack-r-warren Jun 15, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/rawls/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: v2
name: rawls
version: 0.8.0
version: 0.8.1
appVersion: latest
description: Chart for Rawls service in Terra
type: application
Expand Down
88 changes: 88 additions & 0 deletions charts/rawls/templates/migration/job.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
{{- if .Values.migration.enabled }}
{{- $imageTag := .Values.migration.imageTag | default .Values.global.applicationVersion -}}
apiVersion: batch/v1
kind: Job
metadata:
name: rawls-liquibase-migration
annotations:
argocd.argoproj.io/hook: PreSync
argocr.argoproj.io/hook-delete-policy: BeforeHookCreation
labels:
{{- include "rawls.labels" . | nindent 4 }}
jack-r-warren marked this conversation as resolved.
Show resolved Hide resolved
spec:
backoffLimit: 2
template:
metadata:
name: rawls-liquibase-migration
labels:
{{- include "rawls.labels" . | nindent 8 }}
spec:
# Allow migration-liquibase to kill migration-sqlproxy's process
shareProcessNamespace: true
restartPolicy: Never
serviceAccountName: rawls-migration-sa
hostAliases:
- ip: 127.0.0.1
hostnames:
- sqlproxy
volumes:
- name: app-ctmpls
secret:
secretName: {{ .Values.migration.secretPrefix }}-app-ctmpls
- name: sqlproxy-ctmpls
secret:
secretName: {{ .Values.migration.secretPrefix }}-sqlproxy-ctmpls
containers:
- name: migration-liquibase
jack-r-warren marked this conversation as resolved.
Show resolved Hide resolved
image: "gcr.io/broad-dsp-gcr-public/rawls:{{ $imageTag }}"
command: ['bash', '-c']
# Sleep for 15s to allow CloudSQL proxy time to start up. See DDO-1284 / BT-296
# The `find /rawls -name 'rawls*.jar'` is from Rawls's own Dockerfile CMD
# References templated liquibase.properties, see https://docs.google.com/document/d/19ethQWyH29H-jUWwgFoCxKfjmzcG-NCmSgXNAUJAYaU/edit#
args:
- |-
sleep 15s && \
java -cp $(find /rawls -name 'rawls*.jar') liquibase.integration.commandline.Main \
--defaultsFile='/etc/liquibase.properties' \
--classpath="$(find /rawls -name 'rawls*.jar')" \
--username="$DB_USERNAME" \
--password="$DB_PASSWORD" \
{{ .Values.migration.dryRun | ternary "updateSQL" "update" }}; \
EXIT={{ .Values.migration.failBasedOnLiquibase | ternary "$?" "0" }}; \
pkill -SIGTERM cloud_sql_proxy; \
exit $EXIT
env:
- name: DB_USERNAME
valueFrom:
secretKeyRef:
name: rawls-sql-secrets
key: db-username
- name: DB_PASSWORD
valueFrom:
secretKeyRef:
name: rawls-sql-secrets
key: db-password
volumeMounts:
- mountPath: /etc/liquibase.properties
subPath: liquibase.properties
name: app-ctmpls
readOnly: true
- name: migration-sqlproxy
# alpine provides `sh`
image: gcr.io/cloudsql-docker/gce-proxy:1.23.0-alpine
envFrom:
- secretRef:
name: {{ .Values.migration.secretPrefix }}-sqlproxy-env
volumeMounts:
- mountPath: /etc/sqlproxy-service-account.json
subPath: sqlproxy-service-account.json
name: sqlproxy-ctmpls
readOnly: true
command: ['sh', '-c']
args:
- |-
/cloud_sql_proxy ${CLOUDSQL_LOGGING:-"-verbose"} \
jack-r-warren marked this conversation as resolved.
Show resolved Hide resolved
-max_connections=${CLOUDSQL_MAXCONNS:-0} \
-instances="${CLOUDSQL_CONNECTION_LIST:-${GOOGLE_PROJECT}:${CLOUDSQL_ZONE}:${CLOUDSQL_INSTANCE}=tcp:0.0.0.0:${PORT:-3306}}" \
-credential_file=${CLOUDSQL_CREDENTIAL_FILE:-"/etc/sqlproxy-service-account.json"}
{{- end -}}
18 changes: 18 additions & 0 deletions charts/rawls/templates/migration/role.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{{- if .Values.migration.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need separate RBAC resources for this job? I think the permissions will be very similar to whatever the Rawls application uses. (Apologies if there's already been some background discussion here that I missed!)

Seems like we could add PreSync to the existing resources to make sure they exist when the job is run, but not delete them before every sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't strictly need separate RBAC, but we do need them to exist, and unfortunately: argoproj/argo-cd#3502 (comment)

Argo CD hooks are not considered part of Argo CD application resources, and features like drift detection and health assessments do not work. So the solution should involve not having to resort to adding "helm.sh/hook": pre-install to a Namespace resource.

Despite the clutter that this involves (#387 (comment)) I don't know of a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment offer a viable workaround? argoproj/argo-cd#3502 (comment)

I.e.

  • RBAC resources sync in wave -2
  • Job syncs in wave -1 with a Sync hook
  • Everything else syncs in default wave 0

If that won't work, I'm fine with duplicating the RBAC resources!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't seen that, that's a good idea. I'd talked to Mike and just now put the RBAC stuff behind a flag so the job can reuse the existing one. I think the only downside to leaning on sync waves is that the normal rbac resources would need to have that sort of migration-specific attribute, but that's a small downside imo. Let's chat at standup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked!

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: rawls-migration-role
labels:
{{- include "rawls.labels" . | nindent 4 }}
annotations:
argocd.argoproj.io/hook: PreSync
argocr.argoproj.io/hook-delete-policy: BeforeHookCreation
argocd.argoproj.io/sync-wave: "-1"
jack-r-warren marked this conversation as resolved.
Show resolved Hide resolved
rules:
- apiGroups: ['policy']
resources: ['podsecuritypolicies']
verbs: ['use']
resourceNames:
- terra-default-psp
{{- end -}}
19 changes: 19 additions & 0 deletions charts/rawls/templates/migration/roleBinding.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{{- if .Values.migration.enabled }}
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: rawls-migration-sa-binding
labels:
{{- include "rawls.labels" . | nindent 4 }}
annotations:
argocd.argoproj.io/hook: PreSync
argocr.argoproj.io/hook-delete-policy: BeforeHookCreation
argocd.argoproj.io/sync-wave: "-1"
subjects:
- kind: ServiceAccount
name: rawls-migration-sa
roleRef:
kind: Role
name: rawls-migration-role
apiGroup: rbac.authorization.k8s.io
{{- end -}}
23 changes: 23 additions & 0 deletions charts/rawls/templates/migration/secretDefinition.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{{- if .Values.migration.enabled }}
apiVersion: secrets-manager.tuenti.io/v1alpha1
kind: SecretDefinition
metadata:
name: rawls-migration-secretdef
labels:
{{- include "rawls.labels" . | nindent 4 }}
annotations:
argocd.argoproj.io/hook: PreSync
argocr.argoproj.io/hook-delete-policy: BeforeHookCreation
argocd.argoproj.io/sync-wave: "-1"
spec:
name: rawls-sql-secrets
keysMap:
db-username:
key: {{ .Values.vault.migration.dbUsernameKey }}
path: {{ required "A valid vault.migration.path is required" .Values.vault.migration.path }}
encoding: text
db-password:
key: {{ .Values.vault.migration.dbPasswordKey }}
path: {{ required "A valid vault.migration.path is required" .Values.vault.migration.path }}
encoding: text
{{- end -}}
12 changes: 12 additions & 0 deletions charts/rawls/templates/migration/serviceAccount.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{{- if .Values.migration.enabled }}
apiVersion: v1
kind: ServiceAccount
metadata:
name: rawls-migration-sa
labels:
{{- include "rawls.labels" . | nindent 4 }}
annotations:
argocd.argoproj.io/hook: PreSync
argocr.argoproj.io/hook-delete-policy: BeforeHookCreation
argocd.argoproj.io/sync-wave: "-1"
{{- end -}}
27 changes: 26 additions & 1 deletion charts/rawls/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ deploymentDefaults:
# Example: `"rawls1-reader"`
name: null
# deploymentDefaults.imageTag -- Image tag to be used when deploying Pods
# @defautl global.applicationVersion
# @default global.applicationVersion
imageTag: null
# deploymentDefaults.replicas -- Number of replicas for the deployment
replicas: 0
Expand Down Expand Up @@ -92,3 +92,28 @@ deploymentDefaults:
periodSeconds: 10
failureThreshold: 1080 # 3 hours before restarted, to prevent liveness probes from interrupting long-running liquibase migrations
successThreshold: 1

vault:
# Migration credentials only referenced if migration.enabled == true
migration:
# vault.migration.path -- Vault path to secret containing DB credentials
path: null
# vault.migration.dbUsernameKey -- Key in Vault secret to DB username
dbUsernameKey: "slick_db_user"
# vault.migration.DbPasswordKey -- Key in Vault secret to DB password
dbPasswordKey: "slick_db_password"

migration:
# migration.enabled -- Whether to run a Liquibase migration job pre-sync
enabled: false
# migration.dryRun -- When true, merely print migration SQL; when false, execute it
dryRun: true
# migration.failBasedOnLiquibase -- When true, fail the job (and ArgoCD sync!) if the Liquibase command fails
failBasedOnLiquibase: true
# migration.imageTag -- Override the image tag to run the migration on
# @default global.applicationVersion
# WARNING: App may behave unexpectedly if its database has been migrated to a different version than the app
imageTag: null
# migration.secretPrefix -- Prefix for ctmpl and env secrets
# NOTE: Generally equals some deploymentDefaults.name, as secrets are per-deployment but migrations are per-namespace
secretPrefix: "rawls-backend"