From ff98acdb1d539d7eefafcc0fd2c1d78442328e5a Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 7 Feb 2020 14:13:58 -0500 Subject: [PATCH 01/17] Add prop for defaultVisible to the ToggleMenu vue component --- js/components/toggle_menu.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/js/components/toggle_menu.js b/js/components/toggle_menu.js index e17a201a..6c23ce06 100644 --- a/js/components/toggle_menu.js +++ b/js/components/toggle_menu.js @@ -5,6 +5,13 @@ export default { mixins: [ToggleMixin], + props: { + defaultVisible: { + type: Boolean, + default: false, + }, + }, + methods: { toggle: function(e) { if (this.$el.contains(e.target)) { From a9d26d26e65da6f1ef7de6058de700cd5f1b945a Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Fri, 7 Feb 2020 14:28:44 -0500 Subject: [PATCH 02/17] Prevent double submitting forms when Save buttons are double clicked --- js/mixins/form.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/js/mixins/form.js b/js/mixins/form.js index 45aa6261..070466de 100644 --- a/js/mixins/form.js +++ b/js/mixins/form.js @@ -15,6 +15,7 @@ export default { return { changed: this.hasChanges, valid: false, + submitted: false, } }, @@ -36,15 +37,16 @@ export default { handleSubmit: function(event) { if (!this.valid) { event.preventDefault() + this.submitted = true } }, }, computed: { canSave: function() { - if (this.changed && this.valid) { + if (this.changed && this.valid && !this.submitted) { return true - } else if (this.enableSave && this.valid) { + } else if (this.enableSave && this.valid && !this.submitted) { return true } else { return false From 46643f7f418a10c71d84c5d5052fab6fede08aa9 Mon Sep 17 00:00:00 2001 From: dandds Date: Sun, 26 Jan 2020 13:44:58 -0500 Subject: [PATCH 03/17] Config for JEDI dev cluster. - Transition to VMSS identity for flexvol - Update some environment variables for cloudzero dev - Overlay for applying migrations - Updates to disable CDN, which will not be available - Removes CronJob for resetting the database; don't need that in this cluster for now. --- deploy/azure/kustomization.yaml | 1 - deploy/overlays/cloudzero-dev/envvars.yml | 23 +++++++--- deploy/overlays/cloudzero-dev/flex_vol.yml | 41 ++++++++--------- .../overlays/cloudzero-dev/kustomization.yaml | 3 +- deploy/overlays/cloudzero-dev/namespace.yml | 2 +- deploy/overlays/cloudzero-dev/ports.yml | 4 +- .../overlays/cloudzero-dev/reset-cron-job.yml | 46 ------------------- .../kustomization.yaml | 5 ++ .../migration-cloudzero-dev/migration.yaml | 16 +++++++ deploy/shared/kustomization.yaml | 3 ++ script/k8s_config | 1 + 11 files changed, 66 insertions(+), 79 deletions(-) delete mode 100644 deploy/overlays/cloudzero-dev/reset-cron-job.yml create mode 100644 deploy/overlays/migration-cloudzero-dev/kustomization.yaml create mode 100644 deploy/overlays/migration-cloudzero-dev/migration.yaml create mode 100644 deploy/shared/kustomization.yaml diff --git a/deploy/azure/kustomization.yaml b/deploy/azure/kustomization.yaml index d0162394..b46021b0 100644 --- a/deploy/azure/kustomization.yaml +++ b/deploy/azure/kustomization.yaml @@ -10,6 +10,5 @@ resources: - volume-claim.yml - nginx-client-ca-bundle.yml - acme-challenges.yml - - aadpodidentity.yml - nginx-snippets.yml - autoscaling.yml diff --git a/deploy/overlays/cloudzero-dev/envvars.yml b/deploy/overlays/cloudzero-dev/envvars.yml index 179811ed..ded47f7b 100644 --- a/deploy/overlays/cloudzero-dev/envvars.yml +++ b/deploy/overlays/cloudzero-dev/envvars.yml @@ -4,19 +4,30 @@ kind: ConfigMap metadata: name: atst-worker-envvars data: + AZURE_ACCOUNT_NAME: jeditasksatat CELERY_DEFAULT_QUEUE: celery-staging - SERVER_NAME: staging.atat.code.mil FLASK_ENV: staging + PGDATABASE: cloudzero_jedidev_atat + PGHOST: 191.238.6.43 + PGUSER: atat@cloudzero-jedidev-sql + PGSSLMODE: require + REDIS_HOST: 10.1.3.34:6380 + SERVER_NAME: dev.atat.cloud.mil --- apiVersion: v1 kind: ConfigMap metadata: name: atst-envvars data: - ASSETS_URL: https://atat-cdn-staging.azureedge.net/ - CDN_ORIGIN: https://staging.atat.code.mil + ASSETS_URL: "" + AZURE_ACCOUNT_NAME: jeditasksatat + CAC_URL: https://auth-dev.atat.cloud.mil + CDN_ORIGIN: https://dev.atat.cloud.mil CELERY_DEFAULT_QUEUE: celery-staging FLASK_ENV: staging - STATIC_URL: https://atat-cdn-staging.azureedge.net/static/ - PGHOST: cloudzero-dev-sql.postgres.database.azure.com - REDIS_HOST: cloudzero-dev-redis.redis.cache.windows.net:6380 + PGDATABASE: cloudzero_jedidev_atat + PGHOST: 191.238.6.43 + PGUSER: atat@cloudzero-jedidev-sql + PGSSLMODE: require + REDIS_HOST: 10.1.3.34:6380 + SESSION_COOKIE_DOMAIN: atat.cloud.mil diff --git a/deploy/overlays/cloudzero-dev/flex_vol.yml b/deploy/overlays/cloudzero-dev/flex_vol.yml index a3c65df7..990d21e5 100644 --- a/deploy/overlays/cloudzero-dev/flex_vol.yml +++ b/deploy/overlays/cloudzero-dev/flex_vol.yml @@ -9,23 +9,19 @@ spec: - name: nginx-secret flexVolume: options: - keyvaultname: "cloudzero-dev-keyvault" - # keyvaultobjectnames: "dhparam4096;cert;cert" - keyvaultobjectnames: "foo" - keyvaultobjectaliases: "FOO" - keyvaultobjecttypes: "secret" - usevmmanagedidentity: "true" usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "dhparam4096;ATATCERT;ATATCERT" - name: flask-secret flexVolume: options: - keyvaultname: "cloudzero-dev-keyvault" - # keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" - keyvaultobjectnames: "master-PGPASSWORD" - keyvaultobjectaliases: "PGPASSWORD" - keyvaultobjecttypes: "secret" - usevmmanagedidentity: "true" usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" --- apiVersion: extensions/v1beta1 kind: Deployment @@ -38,10 +34,11 @@ spec: - name: flask-secret flexVolume: options: - keyvaultname: "cloudzero-dev-keyvault" - keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" - usevmmanagedidentity: "true" usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" --- apiVersion: extensions/v1beta1 kind: Deployment @@ -54,10 +51,11 @@ spec: - name: flask-secret flexVolume: options: - keyvaultname: "cloudzero-dev-keyvault" - keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" - usevmmanagedidentity: "true" usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" --- apiVersion: batch/v1beta1 kind: CronJob @@ -72,7 +70,8 @@ spec: - name: flask-secret flexVolume: options: - keyvaultname: "cloudzero-dev-keyvault" - keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" - usevmmanagedidentity: "true" usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" diff --git a/deploy/overlays/cloudzero-dev/kustomization.yaml b/deploy/overlays/cloudzero-dev/kustomization.yaml index 24705531..65262fbe 100644 --- a/deploy/overlays/cloudzero-dev/kustomization.yaml +++ b/deploy/overlays/cloudzero-dev/kustomization.yaml @@ -1,9 +1,8 @@ -namespace: staging +namespace: cloudzero-dev bases: - ../../azure/ resources: - namespace.yml - - reset-cron-job.yml patchesStrategicMerge: - ports.yml - envvars.yml diff --git a/deploy/overlays/cloudzero-dev/namespace.yml b/deploy/overlays/cloudzero-dev/namespace.yml index ee38adfb..242c3a2f 100644 --- a/deploy/overlays/cloudzero-dev/namespace.yml +++ b/deploy/overlays/cloudzero-dev/namespace.yml @@ -1,4 +1,4 @@ apiVersion: v1 kind: Namespace metadata: - name: staging + name: cloudzero-dev diff --git a/deploy/overlays/cloudzero-dev/ports.yml b/deploy/overlays/cloudzero-dev/ports.yml index 8dbbd0f1..5225cf3c 100644 --- a/deploy/overlays/cloudzero-dev/ports.yml +++ b/deploy/overlays/cloudzero-dev/ports.yml @@ -5,7 +5,7 @@ metadata: name: atst-main annotations: service.beta.kubernetes.io/azure-load-balancer-internal: "true" - service.beta.kubernetes.io/azure-load-balancer-internal-subnet: "cloudzero-dev-public" + service.beta.kubernetes.io/azure-load-balancer-internal-subnet: "cloudzero-jedidev-public" spec: loadBalancerIP: "" ports: @@ -22,7 +22,7 @@ metadata: name: atst-auth annotations: service.beta.kubernetes.io/azure-load-balancer-internal: "true" - service.beta.kubernetes.io/azure-load-balancer-internal-subnet: "cloudzero-dev-public" + service.beta.kubernetes.io/azure-load-balancer-internal-subnet: "cloudzero-jedidev-public" spec: loadBalancerIP: "" ports: diff --git a/deploy/overlays/cloudzero-dev/reset-cron-job.yml b/deploy/overlays/cloudzero-dev/reset-cron-job.yml deleted file mode 100644 index b4792e5d..00000000 --- a/deploy/overlays/cloudzero-dev/reset-cron-job.yml +++ /dev/null @@ -1,46 +0,0 @@ -apiVersion: batch/v1beta1 -kind: CronJob -metadata: - name: reset-db - namespace: atat -spec: - schedule: "0 4 * * *" - concurrencyPolicy: Replace - successfulJobsHistoryLimit: 1 - jobTemplate: - spec: - template: - metadata: - labels: - app: atst - role: reset-db - aadpodidbinding: atat-kv-id-binding - spec: - restartPolicy: OnFailure - containers: - - name: reset - image: $CONTAINER_IMAGE - command: [ - "/bin/sh", "-c" - ] - args: [ - "/opt/atat/atst/.venv/bin/python", - "/opt/atat/atst/script/reset_database.py" - ] - envFrom: - - configMapRef: - name: atst-worker-envvars - volumeMounts: - - name: flask-secret - mountPath: "/config" - volumes: - - name: flask-secret - flexVolume: - driver: "azure/kv" - options: - usepodidentity: "true" - keyvaultname: "atat-vault-test" - keyvaultobjectnames: "staging-AZURE-STORAGE-KEY;staging-MAIL-PASSWORD;staging-PGPASSWORD;staging-REDIS-PASSWORD;staging-SECRET-KEY" - keyvaultobjectaliases: "AZURE_STORAGE_KEY;MAIL_PASSWORD;PGPASSWORD;REDIS_PASSWORD;SECRET_KEY" - keyvaultobjecttypes: "secret;secret;secret;secret;key" - tenantid: $TENANT_ID diff --git a/deploy/overlays/migration-cloudzero-dev/kustomization.yaml b/deploy/overlays/migration-cloudzero-dev/kustomization.yaml new file mode 100644 index 00000000..b12c0b88 --- /dev/null +++ b/deploy/overlays/migration-cloudzero-dev/kustomization.yaml @@ -0,0 +1,5 @@ +namespace: cloudzero-dev +bases: + - ../../shared/ +patchesStrategicMerge: + - migration.yaml diff --git a/deploy/overlays/migration-cloudzero-dev/migration.yaml b/deploy/overlays/migration-cloudzero-dev/migration.yaml new file mode 100644 index 00000000..53a39dcc --- /dev/null +++ b/deploy/overlays/migration-cloudzero-dev/migration.yaml @@ -0,0 +1,16 @@ +apiVersion: batch/v1 +kind: Job +metadata: + name: migration +spec: + template: + spec: + volumes: + - name: flask-secret + flexVolume: + options: + usepodidentity: "false" + usevmmanagedidentity: "true" + vmmanagedidentityclientid: $VMSS_CLIENT_ID + keyvaultname: "cz-jedidev-keyvault" + keyvaultobjectnames: "AZURE-STORAGE-KEY;MAIL-PASSWORD;PGPASSWORD;REDIS-PASSWORD;SECRET-KEY" diff --git a/deploy/shared/kustomization.yaml b/deploy/shared/kustomization.yaml new file mode 100644 index 00000000..38dddc7e --- /dev/null +++ b/deploy/shared/kustomization.yaml @@ -0,0 +1,3 @@ +namespace: atat +resources: + - migration.yaml diff --git a/script/k8s_config b/script/k8s_config index b489c942..36dc5ef7 100755 --- a/script/k8s_config +++ b/script/k8s_config @@ -13,6 +13,7 @@ SETTINGS=( AUTH_DOMAIN KV_MI_ID KV_MI_CLIENT_ID + VMSS_CLIENT_ID TENANT_ID ) From 014215155819937d616cab181537f27e68380641 Mon Sep 17 00:00:00 2001 From: dandds Date: Sat, 8 Feb 2020 12:58:18 -0500 Subject: [PATCH 04/17] Database user needs to own tables and sequences. This change allows the newly made database user to apply migrations. It also includes a very Azure-specific change. Say we have an Azure Postgres database user "root", which is the user making the database connections for this script, and it is creating an "atat" user/role. That root user will be a member of the azure_pg_admin group. In order for root to change the ownership of the tables in the database to atat, it needs to have membership in the atat role. To achieve this we grant azure_pg_admin the atat role. --- script/database_setup.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/script/database_setup.py b/script/database_setup.py index 7784be05..e4964516 100644 --- a/script/database_setup.py +++ b/script/database_setup.py @@ -16,16 +16,14 @@ from reset_database import reset_database def database_setup(username, password, dbname, ccpo_users): + print("Applying schema and seeding roles and permissions.") + reset_database() + print( f"Creating Postgres user role for '{username}' and granting all privileges to database '{dbname}'." ) - try: - _create_database_user(username, password, dbname) - except sqlalchemy.exc.ProgrammingError as err: - print(f"Postgres user role '{username}' already exists.") + _create_database_user(username, password, dbname) - print("Applying schema and seeding roles and permissions.") - reset_database() print("Creating initial set of CCPO users.") _add_ccpo_users(ccpo_users) @@ -47,6 +45,22 @@ def _create_database_user(username, password, dbname): f"ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT ALL PRIVILEGES ON FUNCTIONS TO {username}; \n" ) + try: + # TODO: make this more configurable + engine.execute(f"GRANT {username} TO azure_pg_admin;") + except sqlalchemy.exc.ProgrammingError as err: + print(f"Cannot grant new role {username} to azure_pg_admin") + + for table in meta.tables: + engine.execute(f"ALTER TABLE {table} OWNER TO {username};\n") + + sequence_results = engine.execute( + "SELECT c.relname FROM pg_class c WHERE c.relkind = 'S';" + ).fetchall() + sequences = [p[0] for p in sequence_results] + for sequence in sequences: + engine.execute(f"ALTER SEQUENCE {sequence} OWNER TO {username};\n") + trans.commit() From a6377b8d8698057b9eb317ffad03056966c57d2b Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Mon, 10 Feb 2020 11:40:14 -0500 Subject: [PATCH 05/17] Update TO number text to remove references to 13-digits --- translations.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/translations.yaml b/translations.yaml index c16f89c6..b80f5dca 100644 --- a/translations.yaml +++ b/translations.yaml @@ -313,7 +313,7 @@ forms: upload_error: There was an error uploading your file. Please try again. If you encounter repeated problems uploading this file, please contact CCPO. size_error: "The file you have selected is too large. Please choose a file no larger than {file_size_limit}MB." filename_error: File names can only contain the characters A-Z, 0-9, space, hyphen, underscore, and period. - number_description: 13-Digit Task Order Number + number_description: Task Order Number pop_errors: date_order: PoP start date must be before end date. range: Date must be between {start} and {end}. @@ -530,7 +530,7 @@ task_orders: form: add_clin: Add Another CLIN add_to_header: Enter the Task Order number - add_to_description: Please input your 13-digit Task Order number. This number may be listed under "Order Number" if your Contracting Officer used form 1149, or "Delivery Order/Call No." if form 1155 was used. Moving forward, this portion of funding will be referenced by the recorded Task Order number. + add_to_description: Please input your Task Order number. This number may be listed under "Order Number" if your Contracting Officer used form 1149, or "Delivery Order/Call No." if form 1155 was used. Moving forward, this portion of funding will be referenced by the recorded Task Order number. builder_base: cancel_modal: Do you want to save this draft? delete_draft: No, delete it From e4f23a583eff9f12b8d6e80ccd440dadde293bdf Mon Sep 17 00:00:00 2001 From: leigh-mil Date: Thu, 6 Feb 2020 18:36:17 -0500 Subject: [PATCH 06/17] Move cookieName into a variable --- js/components/sidenav_toggler.js | 3 ++- js/lib/constants.js | 1 + js/mixins/expand_sidenav.js | 7 ++++--- 3 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 js/lib/constants.js diff --git a/js/components/sidenav_toggler.js b/js/components/sidenav_toggler.js index 11717849..d545b3ed 100644 --- a/js/components/sidenav_toggler.js +++ b/js/components/sidenav_toggler.js @@ -1,5 +1,6 @@ import ExpandSidenavMixin from '../mixins/expand_sidenav' import ToggleMixin from '../mixins/toggle' +import { sidenavCookieName } from '../lib/constants' export default { name: 'sidenav-toggler', @@ -14,7 +15,7 @@ export default { toggle: function(e) { e.preventDefault() this.isVisible = !this.isVisible - document.cookie = this.cookieName + '=' + this.isVisible + '; path=/' + document.cookie = sidenavCookieName + '=' + this.isVisible + '; path=/' this.$parent.$emit('sidenavToggle', this.isVisible) }, }, diff --git a/js/lib/constants.js b/js/lib/constants.js new file mode 100644 index 00000000..b4de4fcf --- /dev/null +++ b/js/lib/constants.js @@ -0,0 +1 @@ +export const sidenavCookieName = 'expandSidenav' diff --git a/js/mixins/expand_sidenav.js b/js/mixins/expand_sidenav.js index 7553b7d4..2af9c87a 100644 --- a/js/mixins/expand_sidenav.js +++ b/js/mixins/expand_sidenav.js @@ -1,11 +1,12 @@ +import { sidenavCookieName } from '../lib/constants' + export default { props: { - cookieName: 'expandSidenav', defaultVisible: { type: Boolean, default: function() { - if (document.cookie.match(this.cookieName)) { - return !!document.cookie.match(this.cookieName + ' *= *true') + if (document.cookie.match(sidenavCookieName)) { + return !!document.cookie.match(sidenavCookieName + ' *= *true') } else { return true } From 5716e20b9e69e3a6e4081968df7060e621339f5a Mon Sep 17 00:00:00 2001 From: graham-dds Date: Wed, 5 Feb 2020 11:06:50 -0500 Subject: [PATCH 07/17] Edit query body to aggregate by month --- atst/domain/csp/cloud/azure_cloud_provider.py | 2 +- atst/domain/csp/cloud/mock_cloud_provider.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/atst/domain/csp/cloud/azure_cloud_provider.py b/atst/domain/csp/cloud/azure_cloud_provider.py index 1d921fde..2d00dcf4 100644 --- a/atst/domain/csp/cloud/azure_cloud_provider.py +++ b/atst/domain/csp/cloud/azure_cloud_provider.py @@ -1060,7 +1060,7 @@ class AzureCloudProvider(CloudProviderInterface): "timeframe": "Custom", "timePeriod": {"from": payload.from_date, "to": payload.to_date,}, "dataset": { - "granularity": "Daily", + "granularity": "Monthly", "aggregation": {"totalCost": {"name": "PreTaxCost", "function": "Sum"}}, "grouping": [{"type": "Dimension", "name": "InvoiceId"}], }, diff --git a/atst/domain/csp/cloud/mock_cloud_provider.py b/atst/domain/csp/cloud/mock_cloud_provider.py index 7ec0636f..8213ac70 100644 --- a/atst/domain/csp/cloud/mock_cloud_provider.py +++ b/atst/domain/csp/cloud/mock_cloud_provider.py @@ -463,7 +463,7 @@ class MockCloudProvider(CloudProviderInterface): **dict( columns=[ {"name": "PreTaxCost", "type": "Number"}, - {"name": "UsageDate", "type": "Number"}, + {"name": "BillingMonth", "type": "Datetime"}, {"name": "InvoiceId", "type": "String"}, {"name": "Currency", "type": "String"}, ], From 411d8a877cf8b22c2068bb1a726689179295a425 Mon Sep 17 00:00:00 2001 From: graham-dds Date: Wed, 5 Feb 2020 11:10:51 -0500 Subject: [PATCH 08/17] Add sample reporting data to mock cloud provider --- atst/domain/csp/cloud/mock_cloud_provider.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/atst/domain/csp/cloud/mock_cloud_provider.py b/atst/domain/csp/cloud/mock_cloud_provider.py index 8213ac70..da1cccd8 100644 --- a/atst/domain/csp/cloud/mock_cloud_provider.py +++ b/atst/domain/csp/cloud/mock_cloud_provider.py @@ -1,4 +1,5 @@ from uuid import uuid4 +import pendulum from .cloud_provider_interface import CloudProviderInterface from .exceptions import ( @@ -459,6 +460,11 @@ class MockCloudProvider(CloudProviderInterface): self._maybe_raise(self.UNAUTHORIZED_RATE, self.AUTHORIZATION_EXCEPTION) object_id = str(uuid4()) + start_of_month = pendulum.today(tz="utc").start_of("month").replace(tzinfo=None) + this_month = start_of_month.to_atom_string() + last_month = start_of_month.subtract(months=1).to_atom_string() + two_months_ago = start_of_month.subtract(months=2).to_atom_string() + properties = CostManagementQueryProperties( **dict( columns=[ @@ -467,7 +473,13 @@ class MockCloudProvider(CloudProviderInterface): {"name": "InvoiceId", "type": "String"}, {"name": "Currency", "type": "String"}, ], - rows=[], + rows=[ + [1.0, two_months_ago, "", "USD"], + [500.0, two_months_ago, "e05009w9sf", "USD"], + [50.0, last_month, "", "USD"], + [1000.0, last_month, "e0500a4qhw", "USD"], + [500.0, this_month, "", "USD"], + ], ) ) From 2f1c57aef4554f1a139c151ee6a90795220bc429 Mon Sep 17 00:00:00 2001 From: graham-dds Date: Wed, 5 Feb 2020 14:20:02 -0500 Subject: [PATCH 09/17] Add fn to prepare Azure reporting data for views --- atst/domain/csp/reports.py | 24 +++++++++++ tests/domain/cloud/reports/test_reports.py | 47 +++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/atst/domain/csp/reports.py b/atst/domain/csp/reports.py index 3f9ccbf8..0e29c5ff 100644 --- a/atst/domain/csp/reports.py +++ b/atst/domain/csp/reports.py @@ -1,6 +1,7 @@ from collections import defaultdict import json from decimal import Decimal +import pendulum def load_fixture_data(): @@ -136,3 +137,26 @@ class MockReportingProvider: CLIN_spend_dict[clin]["invoiced"] += Decimal(spend) return CLIN_spend_dict return {} + + +def prepare_azure_reporting_data(rows: list): + """ + Returns a dict representing invoiced and estimated funds for a portfolio given + a list of rows from CostManagementQueryCSPResult.properties.rows + { + invoiced: Decimal, + estimated: Decimal + } + """ + + estimated = [] + while rows: + if pendulum.parse(rows[-1][1]) >= pendulum.now(tz="utc").start_of("month"): + estimated.append(rows.pop()) + else: + break + + return dict( + invoiced=Decimal(sum([row[0] for row in rows])), + estimated=Decimal(sum([row[0] for row in estimated])), + ) diff --git a/tests/domain/cloud/reports/test_reports.py b/tests/domain/cloud/reports/test_reports.py index 1e74e9b9..4c85a9bc 100644 --- a/tests/domain/cloud/reports/test_reports.py +++ b/tests/domain/cloud/reports/test_reports.py @@ -1,5 +1,7 @@ -from atst.domain.csp.reports import MockReportingProvider +from atst.domain.csp.reports import MockReportingProvider, prepare_azure_reporting_data from tests.factories import PortfolioFactory +from decimal import Decimal +import pendulum def test_get_environment_monthly_totals(): @@ -56,3 +58,46 @@ def test_get_application_monthly_totals(): assert totals["last_month"] == 700 assert totals["total"] == 2500 assert [env["name"] for env in totals["environments"]] == ["A", "Z"] + + +class TestPrepareAzureData: + start_of_month = pendulum.today(tz="utc").start_of("month").replace(tzinfo=None) + next_month = start_of_month.add(months=1).to_atom_string() + this_month = start_of_month.to_atom_string() + last_month = start_of_month.subtract(months=1).to_atom_string() + two_months_ago = last_month = start_of_month.subtract(months=2).to_atom_string() + + def test_estimated_and_invoiced(self): + rows = [ + [150.0, self.two_months_ago, "", "USD"], + [100.0, self.last_month, "e0500a4qhw", "USD"], + [50.0, self.this_month, "", "USD"], + [50.0, self.next_month, "", "USD"], + ] + output = prepare_azure_reporting_data(rows) + + assert output.get("invoiced") == Decimal(250.0) + assert output.get("estimated") == Decimal(100.0) + + def test_just_estimated(self): + rows = [ + [100.0, self.this_month, "", "USD"], + ] + output = prepare_azure_reporting_data(rows) + + assert output.get("invoiced") == Decimal(0.0) + assert output.get("estimated") == Decimal(100.0) + + def test_just_invoiced(self): + rows = [ + [100.0, self.last_month, "", "USD"], + ] + output = prepare_azure_reporting_data(rows) + + assert output.get("invoiced") == Decimal(100.0) + assert output.get("estimated") == Decimal(0.0) + + def test_no_rows(self): + output = prepare_azure_reporting_data([]) + assert output.get("invoiced") == Decimal(0.0) + assert output.get("estimated") == Decimal(0.0) From 55736b723eb221d1d8bfa789d154780d62a27fbf Mon Sep 17 00:00:00 2001 From: graham-dds Date: Wed, 5 Feb 2020 15:04:28 -0500 Subject: [PATCH 10/17] Move calc of a portfolio's obligated funds to prop Add a property on the portfolio model to calculate the total obligated funds for a portfolio. This replaces a one-off calculation in a view function, and sets up functionality for future access --- atst/models/portfolio.py | 6 ++++++ atst/routes/portfolios/index.py | 10 ++-------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/atst/models/portfolio.py b/atst/models/portfolio.py index 5a8f0f1e..2ddcaa41 100644 --- a/atst/models/portfolio.py +++ b/atst/models/portfolio.py @@ -89,6 +89,12 @@ class Portfolio( def active_task_orders(self): return [task_order for task_order in self.task_orders if task_order.is_active] + @property + def total_obligated_funds(self): + return sum( + (task_order.total_obligated_funds for task_order in self.active_task_orders) + ) + @property def funding_duration(self): """ diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index f9e7d5cf..c0fea654 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -40,17 +40,11 @@ def reports(portfolio_id): if any(map(lambda clin: clin["remaining"] < 0, current_obligated_funds)): flash("insufficient_funds") - # wrapped in str() because the sum of obligated funds returns a Decimal object - total_portfolio_value = str( - sum( - task_order.total_obligated_funds - for task_order in portfolio.active_task_orders - ) - ) return render_template( "portfolios/reports/index.html", portfolio=portfolio, - total_portfolio_value=total_portfolio_value, + # wrapped in str() because the sum of obligated funds returns a Decimal object + total_portfolio_value=str(portfolio.total_obligated_funds), current_obligated_funds=current_obligated_funds, expired_task_orders=Reports.expired_task_orders(portfolio), monthly_spending=Reports.monthly_spending(portfolio), From baa3f390d278584f8828dcc94a9d2e9127ded077 Mon Sep 17 00:00:00 2001 From: graham-dds Date: Thu, 6 Feb 2020 14:05:52 -0500 Subject: [PATCH 11/17] Extend graph width filter to handle 0/0 --- atst/filters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atst/filters.py b/atst/filters.py index 3508f1e9..84191017 100644 --- a/atst/filters.py +++ b/atst/filters.py @@ -5,7 +5,7 @@ from flask import render_template from jinja2 import contextfilter from jinja2.exceptions import TemplateNotFound from urllib.parse import urlparse, urlunparse, parse_qs, urlencode -from decimal import DivisionByZero as DivisionByZeroException +from decimal import DivisionByZero as DivisionByZeroException, InvalidOperation def iconSvg(name): @@ -43,7 +43,7 @@ def obligatedFundingGraphWidth(values): numerator, denominator = values try: return (numerator / denominator) * 100 - except DivisionByZeroException: + except (DivisionByZeroException, InvalidOperation): return 0 From 63cb05249f576bf641b38de3260f827cd213708b Mon Sep 17 00:00:00 2001 From: graham-dds Date: Thu, 6 Feb 2020 14:45:05 -0500 Subject: [PATCH 12/17] Add Reports domain method to get portfolio spend --- atst/domain/reports.py | 24 ++++++++++++++++++++++++ tests/domain/test_reports.py | 35 +++++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/atst/domain/reports.py b/atst/domain/reports.py index 99b229e3..30996290 100644 --- a/atst/domain/reports.py +++ b/atst/domain/reports.py @@ -1,5 +1,11 @@ from flask import current_app from itertools import groupby +from atst.domain.csp.cloud.models import ( + ReportingCSPPayload, + CostManagementQueryCSPResult, +) +from atst.domain.csp.reports import prepare_azure_reporting_data +import pendulum class Reports: @@ -42,3 +48,21 @@ class Reports: } ) return output + + @classmethod + def get_portfolio_spending(cls, portfolio): + # TODO: Extend this function to make from_date and to_date configurable + from_date = pendulum.now().subtract(years=1).add(days=1).format("YYYY-MM-DD") + to_date = pendulum.now().format("YYYY-MM-DD") + rows = [] + + if portfolio.csp_data: + payload = ReportingCSPPayload( + from_date=from_date, to_date=to_date, **portfolio.csp_data + ) + response: CostManagementQueryCSPResult = current_app.csp.cloud.get_reporting_data( + payload + ) + rows = response.properties.rows + + return prepare_azure_reporting_data(rows) diff --git a/tests/domain/test_reports.py b/tests/domain/test_reports.py index 33ac926e..cdd5de5e 100644 --- a/tests/domain/test_reports.py +++ b/tests/domain/test_reports.py @@ -1,8 +1,31 @@ -# TODO: Implement when we get real reporting data -def test_expired_task_orders(): - pass +import pytest + +from atst.domain.reports import Reports +from tests.factories import PortfolioFactory +from decimal import Decimal -# TODO: Implement when we get real reporting data -def test_obligated_funds_by_JEDI_clin(): - pass +@pytest.fixture(scope="function") +def portfolio(): + portfolio = PortfolioFactory.create() + return portfolio + + +class TestGetPortfolioSpending: + csp_data = { + "tenant_id": "", + "billing_profile_properties": { + "invoice_sections": [{"invoice_section_id": "",}] + }, + } + + def test_with_csp_data(self, portfolio): + portfolio.csp_data = self.csp_data + data = Reports.get_portfolio_spending(portfolio) + assert data["invoiced"] == Decimal(1551.0) + assert data["estimated"] == Decimal(500.0) + + def test_without_csp_data(self, portfolio): + data = Reports.get_portfolio_spending(portfolio) + assert data["invoiced"] == Decimal(0) + assert data["estimated"] == Decimal(0) From 4a78aa07c9d39455a923e8fa3ed9975f60e05113 Mon Sep 17 00:00:00 2001 From: graham-dds Date: Thu, 6 Feb 2020 14:45:44 -0500 Subject: [PATCH 13/17] Remove non-MVP reporting elements - Edits views to only show spending at the portfolio level -- no longer broken out by JEDI CLIN - Removes Monthly Spending table -- keeps the template, just no longer uses it. - Removes amount unspent from the expired funding table --- atst/routes/portfolios/index.py | 12 ++- .../reports/expired_task_orders.html | 8 +- templates/portfolios/reports/index.html | 2 - .../portfolios/reports/obligated_funds.html | 93 +++++++++---------- 4 files changed, 56 insertions(+), 59 deletions(-) diff --git a/atst/routes/portfolios/index.py b/atst/routes/portfolios/index.py index c0fea654..795d4b70 100644 --- a/atst/routes/portfolios/index.py +++ b/atst/routes/portfolios/index.py @@ -34,10 +34,17 @@ def create_portfolio(): @user_can(Permissions.VIEW_PORTFOLIO_REPORTS, message="view portfolio reports") def reports(portfolio_id): portfolio = Portfolios.get(g.current_user, portfolio_id) + spending = Reports.get_portfolio_spending(portfolio) + obligated = portfolio.total_obligated_funds + remaining = obligated - (spending["invoiced"] + spending["estimated"]) - current_obligated_funds = Reports.obligated_funds_by_JEDI_clin(portfolio) + current_obligated_funds = { + **spending, + "obligated": obligated, + "remaining": remaining, + } - if any(map(lambda clin: clin["remaining"] < 0, current_obligated_funds)): + if current_obligated_funds["remaining"] < 0: flash("insufficient_funds") return render_template( @@ -47,6 +54,5 @@ def reports(portfolio_id): total_portfolio_value=str(portfolio.total_obligated_funds), current_obligated_funds=current_obligated_funds, expired_task_orders=Reports.expired_task_orders(portfolio), - monthly_spending=Reports.monthly_spending(portfolio), retrieved=datetime.now(), # mocked datetime of reporting data retrival ) diff --git a/templates/portfolios/reports/expired_task_orders.html b/templates/portfolios/reports/expired_task_orders.html index dcde6683..f55bb57e 100644 --- a/templates/portfolios/reports/expired_task_orders.html +++ b/templates/portfolios/reports/expired_task_orders.html @@ -16,13 +16,12 @@ PoP CLIN Value Amount Obligated - Amount Unspent {% for task_order in expired_task_orders %} - + Task Order {{ task_order.number }} {{ Icon("caret_right", classes="icon--tiny icon--blue" ) }} @@ -39,9 +38,8 @@ - {{ clin.end_date | formattedDate(formatter="%b %d, %Y") }} - {{ clin.total_amount | dollars }} - {{ clin.obligated_amount | dollars }} - {{ 0 | dollars }} + {{ clin.total_amount | dollars }} + {{ clin.obligated_amount | dollars }} {% endfor %} {% endfor %} diff --git a/templates/portfolios/reports/index.html b/templates/portfolios/reports/index.html index 747610d9..51364bf7 100644 --- a/templates/portfolios/reports/index.html +++ b/templates/portfolios/reports/index.html @@ -13,7 +13,5 @@
{% include "portfolios/reports/obligated_funds.html" %} {% include "portfolios/reports/expired_task_orders.html" %} -
- {% include "portfolios/reports/application_and_env_spending.html" %} {% endblock %} diff --git a/templates/portfolios/reports/obligated_funds.html b/templates/portfolios/reports/obligated_funds.html index e0f11792..0b41b16f 100644 --- a/templates/portfolios/reports/obligated_funds.html +++ b/templates/portfolios/reports/obligated_funds.html @@ -7,61 +7,56 @@
- {% for JEDI_clin in current_obligated_funds | sort(attribute='name')%} -
-

- {{ "JEDICLINType.{}".format(JEDI_clin.name) | translate }} -

-

Total obligated amount: {{ JEDI_clin.obligated | dollars }}

-
- {% if JEDI_clin.remaining < 0 %} - - {% else %} - {% set invoiced_width = (JEDI_clin.invoiced, JEDI_clin.obligated) | obligatedFundingGraphWidth %} - {% if invoiced_width %} - - - {% endif %} - - {% set estimated_width = (JEDI_clin.estimated, JEDI_clin.obligated) | obligatedFundingGraphWidth %} - {% if estimated_width %} - - - {% endif %} - +
+

+ Total obligated amount: {{ current_obligated_funds.obligated | dollars }} +

+
+ {% if current_obligated_funds.remaining < 0 %} + + {% else %} + {% set invoiced_width = (current_obligated_funds.invoiced, current_obligated_funds.obligated) | obligatedFundingGraphWidth %} + {% if invoiced_width %} + {% endif %} - + {% set estimated_width = (current_obligated_funds.estimated, current_obligated_funds.obligated) | obligatedFundingGraphWidth %} + {% if estimated_width %} + + + {% endif %} + + + {% endif %} +
+
+
+

+ + Invoiced expended funds: +

+

{{ current_obligated_funds.invoiced | dollars }}

-
-
-

- - Invoiced expended funds: -

-

{{ JEDI_clin.invoiced | dollars }}

-
-
-

- - Estimated expended funds: -

-

{{ JEDI_clin.estimated | dollars }}

-
-
-

- 0 else "insufficient"}}"> - Remaining funds: -

-

{{ JEDI_clin.remaining | dollars }}

-
+
+

+ + Estimated expended funds: +

+

{{ current_obligated_funds.estimated | dollars }}

+
+
+

+ 0 else "insufficient"}}"> + Remaining funds: +

+

{{ current_obligated_funds.remaining | dollars }}

- {% endfor %} +

Active Task Orders From 5b60a54dbc39c738033848b7097f2a8b83d377e0 Mon Sep 17 00:00:00 2001 From: graham-dds Date: Thu, 6 Feb 2020 15:42:08 -0500 Subject: [PATCH 14/17] Remove fixture-based reporting methods These methods probably can be reused to handle real Azure reporting data --- atst/domain/csp/reports.py | 127 --------------------- atst/domain/reports.py | 35 ------ tests/domain/cloud/reports/test_reports.py | 58 +--------- 3 files changed, 1 insertion(+), 219 deletions(-) diff --git a/atst/domain/csp/reports.py b/atst/domain/csp/reports.py index 0e29c5ff..700947f7 100644 --- a/atst/domain/csp/reports.py +++ b/atst/domain/csp/reports.py @@ -1,4 +1,3 @@ -from collections import defaultdict import json from decimal import Decimal import pendulum @@ -12,132 +11,6 @@ def load_fixture_data(): class MockReportingProvider: FIXTURE_SPEND_DATA = load_fixture_data() - @classmethod - def get_portfolio_monthly_spending(cls, portfolio): - """ - returns an array of application and environment spending for the - portfolio. Applications and their nested environments are sorted in - alphabetical order by name. - [ - { - name - this_month - last_month - total - environments [ - { - name - this_month - last_month - total - } - ] - } - ] - """ - - fixture_apps = cls.FIXTURE_SPEND_DATA.get(portfolio.name, {}).get( - "applications", [] - ) - - for application in portfolio.applications: - if application.name not in [app["name"] for app in fixture_apps]: - fixture_apps.append({"name": application.name, "environments": []}) - - return sorted( - [ - cls._get_application_monthly_totals(portfolio, fixture_app) - for fixture_app in fixture_apps - if fixture_app["name"] - in [application.name for application in portfolio.applications] - ], - key=lambda app: app["name"], - ) - - @classmethod - def _get_environment_monthly_totals(cls, environment): - """ - returns a dictionary that represents spending totals for an environment e.g. - { - name - this_month - last_month - total - } - """ - return { - "name": environment["name"], - "this_month": sum(environment["spending"]["this_month"].values()), - "last_month": sum(environment["spending"]["last_month"].values()), - "total": sum(environment["spending"]["total"].values()), - } - - @classmethod - def _get_application_monthly_totals(cls, portfolio, fixture_app): - """ - returns a dictionary that represents spending totals for an application - and its environments e.g. - { - name - this_month - last_month - total - environments: [ - { - name - this_month - last_month - total - } - ] - } - """ - application_envs = [ - env - for env in portfolio.all_environments - if env.application.name == fixture_app["name"] - ] - - environments = [ - cls._get_environment_monthly_totals(env) - for env in fixture_app["environments"] - if env["name"] in [e.name for e in application_envs] - ] - - for env in application_envs: - if env.name not in [env["name"] for env in environments]: - environments.append({"name": env.name}) - - return { - "name": fixture_app["name"], - "this_month": sum(env.get("this_month", 0) for env in environments), - "last_month": sum(env.get("last_month", 0) for env in environments), - "total": sum(env.get("total", 0) for env in environments), - "environments": sorted(environments, key=lambda env: env["name"]), - } - - @classmethod - def get_spending_by_JEDI_clin(cls, portfolio): - """ - returns an dictionary of spending per JEDI CLIN for a portfolio - { - jedi_clin: { - invoiced - estimated - }, - } - """ - if portfolio.name in cls.FIXTURE_SPEND_DATA: - CLIN_spend_dict = defaultdict(lambda: defaultdict(Decimal)) - for application in cls.FIXTURE_SPEND_DATA[portfolio.name]["applications"]: - for environment in application["environments"]: - for clin, spend in environment["spending"]["this_month"].items(): - CLIN_spend_dict[clin]["estimated"] += Decimal(spend) - for clin, spend in environment["spending"]["total"].items(): - CLIN_spend_dict[clin]["invoiced"] += Decimal(spend) - return CLIN_spend_dict - return {} - def prepare_azure_reporting_data(rows: list): """ diff --git a/atst/domain/reports.py b/atst/domain/reports.py index 30996290..fc619649 100644 --- a/atst/domain/reports.py +++ b/atst/domain/reports.py @@ -1,5 +1,4 @@ from flask import current_app -from itertools import groupby from atst.domain.csp.cloud.models import ( ReportingCSPPayload, CostManagementQueryCSPResult, @@ -9,46 +8,12 @@ import pendulum class Reports: - @classmethod - def monthly_spending(cls, portfolio): - return current_app.csp.reports.get_portfolio_monthly_spending(portfolio) - @classmethod def expired_task_orders(cls, portfolio): return [ task_order for task_order in portfolio.task_orders if task_order.is_expired ] - @classmethod - def obligated_funds_by_JEDI_clin(cls, portfolio): - clin_spending = current_app.csp.reports.get_spending_by_JEDI_clin(portfolio) - active_clins = portfolio.active_clins - for jedi_clin, clins in groupby( - active_clins, key=lambda clin: clin.jedi_clin_type - ): - if not clin_spending.get(jedi_clin.name): - clin_spending[jedi_clin.name] = {} - clin_spending[jedi_clin.name]["obligated"] = sum( - clin.obligated_amount for clin in clins - ) - - output = [] - for clin in clin_spending.keys(): - invoiced = clin_spending[clin].get("invoiced", 0) - estimated = clin_spending[clin].get("estimated", 0) - obligated = clin_spending[clin].get("obligated", 0) - remaining = obligated - (invoiced + estimated) - output.append( - { - "name": clin, - "invoiced": invoiced, - "estimated": estimated, - "obligated": obligated, - "remaining": remaining, - } - ) - return output - @classmethod def get_portfolio_spending(cls, portfolio): # TODO: Extend this function to make from_date and to_date configurable diff --git a/tests/domain/cloud/reports/test_reports.py b/tests/domain/cloud/reports/test_reports.py index 4c85a9bc..91079d44 100644 --- a/tests/domain/cloud/reports/test_reports.py +++ b/tests/domain/cloud/reports/test_reports.py @@ -1,65 +1,9 @@ -from atst.domain.csp.reports import MockReportingProvider, prepare_azure_reporting_data +from atst.domain.csp.reports import prepare_azure_reporting_data from tests.factories import PortfolioFactory from decimal import Decimal import pendulum -def test_get_environment_monthly_totals(): - environment = { - "name": "Test Environment", - "spending": { - "this_month": {"JEDI_CLIN_1": 100, "JEDI_CLIN_2": 100}, - "last_month": {"JEDI_CLIN_1": 200, "JEDI_CLIN_2": 200}, - "total": {"JEDI_CLIN_1": 1000, "JEDI_CLIN_2": 1000}, - }, - } - totals = MockReportingProvider._get_environment_monthly_totals(environment) - assert totals == { - "name": "Test Environment", - "this_month": 200, - "last_month": 400, - "total": 2000, - } - - -def test_get_application_monthly_totals(): - portfolio = PortfolioFactory.create( - applications=[ - {"name": "Test Application", "environments": [{"name": "Z"}, {"name": "A"}]} - ], - ) - application = { - "name": "Test Application", - "environments": [ - { - "name": "Z", - "spending": { - "this_month": {"JEDI_CLIN_1": 50, "JEDI_CLIN_2": 50}, - "last_month": {"JEDI_CLIN_1": 150, "JEDI_CLIN_2": 150}, - "total": {"JEDI_CLIN_1": 250, "JEDI_CLIN_2": 250}, - }, - }, - { - "name": "A", - "spending": { - "this_month": {"JEDI_CLIN_1": 100, "JEDI_CLIN_2": 100}, - "last_month": {"JEDI_CLIN_1": 200, "JEDI_CLIN_2": 200}, - "total": {"JEDI_CLIN_1": 1000, "JEDI_CLIN_2": 1000}, - }, - }, - ], - } - - totals = MockReportingProvider._get_application_monthly_totals( - portfolio, application - ) - assert totals["name"] == "Test Application" - assert totals["this_month"] == 300 - assert totals["last_month"] == 700 - assert totals["total"] == 2500 - assert [env["name"] for env in totals["environments"]] == ["A", "Z"] - - class TestPrepareAzureData: start_of_month = pendulum.today(tz="utc").start_of("month").replace(tzinfo=None) next_month = start_of_month.add(months=1).to_atom_string() From e6d5369cb0ca7383a95545f0c2df4d7d2c901c9f Mon Sep 17 00:00:00 2001 From: tomdds Date: Mon, 10 Feb 2020 16:14:42 -0500 Subject: [PATCH 15/17] Ensure credential updates properly merge values. Previously updating the credentials would delete values from the existing crednetials if they weren't also present in the update. This adds a method for merging credentials to the KeyVaultCredentials model and adds tests to both the cloud provider and model. --- atst/domain/csp/cloud/azure_cloud_provider.py | 9 +++----- atst/domain/csp/cloud/models.py | 9 ++++++++ tests/domain/cloud/test_azure_csp.py | 21 +++++++++++++++++++ tests/domain/cloud/test_models.py | 20 ++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/atst/domain/csp/cloud/azure_cloud_provider.py b/atst/domain/csp/cloud/azure_cloud_provider.py index 1d921fde..06d80280 100644 --- a/atst/domain/csp/cloud/azure_cloud_provider.py +++ b/atst/domain/csp/cloud/azure_cloud_provider.py @@ -1,6 +1,5 @@ import json from secrets import token_urlsafe -from typing import Any, Dict from uuid import uuid4 from atst.utils import sha256_hex @@ -1026,12 +1025,10 @@ class AzureCloudProvider(CloudProviderInterface): def update_tenant_creds(self, tenant_id, secret: KeyVaultCredentials): hashed = sha256_hex(tenant_id) - new_secrets = secret.dict() curr_secrets = self._source_tenant_creds(tenant_id) - updated_secrets: Dict[str, Any] = {**curr_secrets.dict(), **new_secrets} - us = KeyVaultCredentials(**updated_secrets) - self.set_secret(hashed, json.dumps(us.dict())) - return us + updated_secrets = curr_secrets.merge_credentials(secret) + self.set_secret(hashed, json.dumps(updated_secrets.dict())) + return updated_secrets def _source_tenant_creds(self, tenant_id) -> KeyVaultCredentials: hashed = sha256_hex(tenant_id) diff --git a/atst/domain/csp/cloud/models.py b/atst/domain/csp/cloud/models.py index 358f7934..25521bb9 100644 --- a/atst/domain/csp/cloud/models.py +++ b/atst/domain/csp/cloud/models.py @@ -417,6 +417,15 @@ class KeyVaultCredentials(BaseModel): return values + def merge_credentials( + self, new_creds: "KeyVaultCredentials" + ) -> "KeyVaultCredentials": + updated_creds = {k: v for k, v in new_creds.dict().items() if v} + old_creds = self.dict() + old_creds.update(updated_creds) + + return KeyVaultCredentials(**old_creds) + class SubscriptionCreationCSPPayload(BaseCSPPayload): display_name: str diff --git a/tests/domain/cloud/test_azure_csp.py b/tests/domain/cloud/test_azure_csp.py index 3a25f849..c89e6a77 100644 --- a/tests/domain/cloud/test_azure_csp.py +++ b/tests/domain/cloud/test_azure_csp.py @@ -25,6 +25,7 @@ from atst.domain.csp.cloud.models import ( CostManagementQueryCSPResult, EnvironmentCSPPayload, EnvironmentCSPResult, + KeyVaultCredentials, PrincipalAdminRoleCSPPayload, PrincipalAdminRoleCSPResult, ProductPurchaseCSPPayload, @@ -938,3 +939,23 @@ def test_create_user(mock_azure: AzureCloudProvider): result = mock_azure.create_user(payload) assert result.id == "id" + + +def test_update_tenant_creds(mock_azure: AzureCloudProvider): + with patch.object( + AzureCloudProvider, "set_secret", wraps=mock_azure.set_secret, + ) as set_secret: + set_secret.return_value = None + existing_secrets = { + "tenant_id": "mytenant", + "tenant_admin_username": "admin", + "tenant_admin_password": "foo", # pragma: allowlist secret + } + mock_azure = mock_get_secret(mock_azure, json.dumps(existing_secrets)) + + mock_new_secrets = KeyVaultCredentials(**MOCK_CREDS) + updated_secret = mock_azure.update_tenant_creds("mytenant", mock_new_secrets) + + assert updated_secret == KeyVaultCredentials( + **{**existing_secrets, **MOCK_CREDS} + ) diff --git a/tests/domain/cloud/test_models.py b/tests/domain/cloud/test_models.py index 10c81293..29bc60cb 100644 --- a/tests/domain/cloud/test_models.py +++ b/tests/domain/cloud/test_models.py @@ -100,6 +100,26 @@ def test_KeyVaultCredentials_enforce_root_creds(): ) +def test_KeyVaultCredentials_merge_credentials(): + old_secret = KeyVaultCredentials( + tenant_id="foo", + tenant_admin_username="bar", + tenant_admin_password="baz", # pragma: allowlist secret + ) + new_secret = KeyVaultCredentials( + tenant_id="foo", tenant_sp_client_id="bip", tenant_sp_key="bop" + ) + + expected_update = KeyVaultCredentials( + tenant_id="foo", + tenant_admin_username="bar", + tenant_admin_password="baz", # pragma: allowlist secret + tenant_sp_client_id="bip", + tenant_sp_key="bop", + ) + assert old_secret.merge_credentials(new_secret) == expected_update + + user_payload = { "tenant_id": "123", "display_name": "Han Solo", From f975249f07e1efd2c98d773f1ca296c97938e4f4 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 10 Feb 2020 16:58:07 -0500 Subject: [PATCH 16/17] Set Redis verification mode for TLS connections. If the app is making a TLS connection to Redis, the new config setting REDIS_SSLMODE determines whether CA verification should be performed. Acceptable values are Python `None` or strings "none", "optional", and "required". --- .secrets.baseline | 4 ++-- atst/app.py | 8 +++++++- config/base.ini | 1 + tests/test_app.py | 16 ++++++++++++++++ 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index a233e4cf..f7145df8 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "^.secrets.baseline$|^.*pgsslrootcert.yml$", "lines": null }, - "generated_at": "2020-01-27T19:24:43Z", + "generated_at": "2020-02-10T21:40:38Z", "plugins_used": [ { "base64_limit": 4.5, @@ -82,7 +82,7 @@ "hashed_secret": "afc848c316af1a89d49826c5ae9d00ed769415f3", "is_secret": false, "is_verified": false, - "line_number": 32, + "line_number": 33, "type": "Secret Keyword" } ], diff --git a/atst/app.py b/atst/app.py index 05578827..db6a09c7 100644 --- a/atst/app.py +++ b/atst/app.py @@ -233,12 +233,18 @@ def make_config(direct_config=None): config.set("default", "DATABASE_URI", database_uri) # Assemble REDIS_URI value + redis_use_tls = config["default"].getboolean("REDIS_TLS") redis_uri = "redis{}://{}:{}@{}".format( # pragma: allowlist secret - ("s" if config["default"].getboolean("REDIS_TLS") else ""), + ("s" if redis_use_tls else ""), (config.get("default", "REDIS_USER") or ""), (config.get("default", "REDIS_PASSWORD") or ""), config.get("default", "REDIS_HOST"), ) + if redis_use_tls: + tls_mode = config.get("default", "REDIS_SSLMODE") + tls_mode_str = tls_mode.lower() if tls_mode else "none" + redis_uri = f"{redis_uri}/?ssl_cert_reqs={tls_mode_str}" + config.set("default", "REDIS_URI", redis_uri) return map_config(config) diff --git a/config/base.ini b/config/base.ini index 1f4c732a..55482741 100644 --- a/config/base.ini +++ b/config/base.ini @@ -38,6 +38,7 @@ PGUSER = postgres PORT=8000 REDIS_HOST=localhost:6379 REDIS_PASSWORD +REDIS_SSLMODE REDIS_TLS=False REDIS_USER SECRET_KEY = change_me_into_something_secret diff --git a/tests/test_app.py b/tests/test_app.py index 937a15e2..21fd8284 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -7,6 +7,7 @@ from atst.app import ( make_crl_validator, apply_config_from_directory, apply_config_from_environment, + make_config, ) @@ -67,3 +68,18 @@ def test_apply_config_from_environment_skips_unknown_settings( monkeypatch.setenv("FLARF", "MAYO") apply_config_from_environment(config_object) assert "FLARF" not in config_object.options("default") + + +class TestMakeConfig: + def test_redis_ssl_connection(self): + config = make_config({"REDIS_TLS": True}) + uri = config.get("REDIS_URI") + assert "rediss" in uri + assert "ssl_cert_reqs" in uri + + def test_non_redis_ssl_connection(self): + config = make_config({"REDIS_TLS": False}) + uri = config.get("REDIS_URI") + assert "rediss" not in uri + assert "redis" in uri + assert "ssl_cert_reqs" not in uri From 788f73b7a9f577c725d816f42a29e367fbd785f5 Mon Sep 17 00:00:00 2001 From: dandds Date: Tue, 11 Feb 2020 06:37:32 -0500 Subject: [PATCH 17/17] Separate config for Celery BROKER_URL. I found out the hard way that, despite the Celery docs saying it will respect settings of "none", "required", etc for the ssl_cert_reqs option on ths broker connection uri, one it's underlying dependencies does not. That dependency, Kombu, requires that the option be set as the string version of one of the constants available on the standard library's ssl module ("CERT_NONE", etc.). This fixes our code to supply slightly variant connection URIs for Celery and for the session library. This change can be reverted when Kombu is updated with the correct behavior. --- atst/app.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/atst/app.py b/atst/app.py index db6a09c7..04aed44d 100644 --- a/atst/app.py +++ b/atst/app.py @@ -157,7 +157,6 @@ def map_config(config): **config["default"], "USE_AUDIT_LOG": config["default"].getboolean("USE_AUDIT_LOG"), "ENV": config["default"]["ENVIRONMENT"], - "BROKER_URL": config["default"]["REDIS_URI"], "DEBUG": config["default"].getboolean("DEBUG"), "DEBUG_MAILER": config["default"].getboolean("DEBUG_MAILER"), "SQLALCHEMY_ECHO": config["default"].getboolean("SQLALCHEMY_ECHO"), @@ -240,12 +239,27 @@ def make_config(direct_config=None): (config.get("default", "REDIS_PASSWORD") or ""), config.get("default", "REDIS_HOST"), ) + celery_uri = redis_uri if redis_use_tls: tls_mode = config.get("default", "REDIS_SSLMODE") tls_mode_str = tls_mode.lower() if tls_mode else "none" redis_uri = f"{redis_uri}/?ssl_cert_reqs={tls_mode_str}" + # TODO: Kombu, one of Celery's dependencies, still requires + # that ssl_cert_reqs be passed as the string version of an + # option on the ssl module. We can clean this up and use + # the REDIS_URI for both when this PR to Kombu is released: + # https://github.com/celery/kombu/pull/1139 + kombu_modes = { + "none": "CERT_NONE", + "required": "CERT_REQUIRED", + "optional": "CERT_OPTIONAL", + } + celery_tls_mode_str = kombu_modes[tls_mode_str] + celery_uri = f"{celery_uri}/?ssl_cert_reqs={celery_tls_mode_str}" + config.set("default", "REDIS_URI", redis_uri) + config.set("default", "BROKER_URL", celery_uri) return map_config(config)