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

Dynamic App Setting + App Config Support #7

Conversation

erikschlegel
Copy link

This feature request comprises of the following changes:

  • Added new caf module app_config responsible for provisioning an Application Configuration resource
  • MSI support for app_config module
  • Standardized naming convention for cosmos module ie cosmos_dbs vs cosmos_db. The naming discrepency was causing an issue when a terraform mutation was dependent on an exported attribute from the cosmos resource (ie writing the cosmos connection string as a KV secret)
  • Added the dynamic_app_settings feature for the app_services and function_app modules. This new parameter allows a CAF developer to provision app settings sourced from Terraform exported attributes (ie keyvault URI, app config endpoint)
  • Exported the system assigned managed identity for the machine_learning resource so we can create provision role assignments for the AML MSI identity.
  • Exporting the cosmos connection string so we can save that as a secret via dynamic_secrets module

Copy link

@brockneedscoffee brockneedscoffee left a comment

Choose a reason for hiding this comment

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

really nice work, just had the one comment/question


output app_config {
value = module.app_config
sensitive = true

Choose a reason for hiding this comment

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

@erikschlegel given the CAF team's guidance on this flag do we still want to move forward with it? They have taken it out of the core modules but use it in the landing zone itself

Copy link
Author

Choose a reason for hiding this comment

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

Makes me really nervous getting rid of the sensitive flag given the cosmos connection string would be exposed in clear text within the state file. Removing this flag seemed like a band aid fix due to upgrading to the latest version of Terraform.

Choose a reason for hiding this comment

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

agreed, just wanted to verify

@brockneedscoffee brockneedscoffee self-requested a review January 22, 2021 16:18
@brockneedscoffee brockneedscoffee merged commit e9d498c into cse-kratos:master Jan 22, 2021
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.

2 participants