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

[Issue] Maybe 'azd down' should remove KEY_VAULT* from .env ? #2217

Closed
1 task done
pamelafox opened this issue May 15, 2023 · 1 comment · Fixed by #2278
Closed
1 task done

[Issue] Maybe 'azd down' should remove KEY_VAULT* from .env ? #2217

pamelafox opened this issue May 15, 2023 · 1 comment · Fixed by #2278
Labels
needs-triage For new issues

Comments

@pamelafox
Copy link
Member

Output from azd version
azd version 0.9.0-beta.2 (commit afa7ac6)

Describe the bug

I just completely took down my resources using 'azd down'. I then ran 'azd up' and got deployment failed:

ERROR: deployment failed: failing invoking action 'provision', planning deployment: planning infrastructure provisioning: creating parameters file: substituting command output inside parameter file: reading secret 'POSTGRESPASSWORD' from vault 'relecloudca5zjvr7-vault': getting key vault secret: Get "https://relecloudca5zjvr7-vault.vault.azure.net/secrets/POSTGRESPASSWORD/?api-version=7.3": dial tcp: lookup relecloudca5zjvr7-vault.vault.azure.net: no such host

I presume that's because my .env still has values for AZURE_KEY_VAULT_NAME and AZURE_KEY_VAULT_ENDPOINT. Perhaps calling 'azd down' should remove those values from the .env file?

I will manually do it, which should work.

To Reproduce

On a repo with ACA support, try azd up, azd down and then azd up:
https://github.com/pamelafox/azure-django-postgres-aca-fork

Expected behavior

I expect it to start from scratch after I've down'ed it all.

Environment

Mac OS X M1 VS Code

@ghost ghost added the needs-triage For new issues label May 15, 2023
@ellismg
Copy link
Member

ellismg commented May 18, 2023

Surprising - The intention was that azd down would remove all the outputs from a previous deploy:

destroyOptions := provisioning.NewDestroyOptions(a.flags.forceDelete, a.flags.purgeDelete)
destroyResult, err := infraManager.Destroy(ctx, destroyOptions)
if err != nil {
return nil, fmt.Errorf("deleting infrastructure: %w", err)
}
// Remove any outputs from the template from the environment since destroying the infrastructure
// invalidated them all.
for outputName := range destroyResult.Outputs {
a.env.DotenvDelete(outputName)
}
if err := a.env.Save(); err != nil {
return nil, fmt.Errorf("saving environment: %w", err)
}

In this case, AZURE_KEY_VAULT_NAME and AZURE_KEY_VAULT_ENDPOINT are outputs from main.bicep, so I would have expected them to be deleted. Perhaps we are not returning the correct set of outputs from the the provider?

ellismg added a commit to ellismg/azure-dev that referenced this issue May 19, 2023
The tool had logic to this, but it had been broken for a while due to
a change in how `Environment.Save()` worked. In Azure#1667 we made a change
such that when you save an environment, we would merge in new values
from the existing .env file into the file we were saving, in case the
.env file had been modified by an external hook.

However, that logic did not take into account that we may have
intentionally removed some values (to explicitly delete them) and if
they already existed in the `.env` files, they should be removed as
part of the save operation.

To address this, we now track the deletes that we have preformed so we
can replay them during saving.

To prevent regressing again we also add an end to end test.

Fixes Azure#2217
ellismg added a commit that referenced this issue May 20, 2023
The tool had logic to this, but it had been broken for a while due to
a change in how `Environment.Save()` worked. In #1667 we made a change
such that when you save an environment, we would merge in new values
from the existing .env file into the file we were saving, in case the
.env file had been modified by an external hook.

However, that logic did not take into account that we may have
intentionally removed some values (to explicitly delete them) and if
they already existed in the `.env` files, they should be removed as
part of the save operation.

To address this, we now track the deletes that we have preformed so we
can replay them during saving.

To prevent regressing again we also add an end to end test.

Fixes #2217

Co-authored-by: Wei Lim <weilim@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage For new issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants