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

Deployment script module - AKV certificates #84

Merged
merged 17 commits into from
Apr 25, 2022
Merged

Conversation

Gordonby
Copy link
Collaborator

@Gordonby Gordonby commented Apr 19, 2022

Description

Adds a new module which leverages Key Vaults ability to create self-signed certificates.
It is also able to use these generated certificates in Azure Application Gateway as either frontend or backend certificates.

The script is derived from what is used in this sample.

Closes #69

Adding a new module

  • A proposal has been submitted and approved.
  • I have included "Closes #{module_proposal_issue_number}" in the PR description.
  • I have run brm validate locally to verify the module files.
  • I have run deployment tests locally to ensure the module is deployable.

@Gordonby
Copy link
Collaborator Author

Closing this down for the moment as i've still got an optimisation to do in the AGW script. oops.

@Gordonby Gordonby closed this Apr 19, 2022
@Gordonby
Copy link
Collaborator Author

Gordonby commented Apr 19, 2022

Re-Opening as the script looks fixed.

I'm anticipating feedback on "what if the AKV is in a different RG to the AGW" - but am interested in other thoughts/feedback before I add parameters for this use case.

I'm also tempted to add in a BatchSize(1) for the script loop due to the Application Gateway occasionally not loving concurrent updates.

@Gordonby Gordonby reopened this Apr 19, 2022
@alex-frankel alex-frankel added the Needs: Triage 🔍 Maintainers need to triage still label Apr 19, 2022
Copy link

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

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

Looks good, but paging @bmoore-msft to check if you know there is no way to accomplish this in bicep.

Also, can you confirm this script is idempotent, meaning if this script executes a second time, the end state (including the outputs) will be the same?

@Gordonby
Copy link
Collaborator Author

Gordonby commented Apr 21, 2022

Some great feedback from @bmoore-msft

I think this module will be best down-scoped to just KeyVault. I'm not sure if we'll need another module in the registry for the AGW use case. I think it might be better suited to going into the Azure Quickstart templates repo as a sample that creates an AGW, and uses this module to create the certificate which is referenced by AGW.

I'll work on the suggested changes, then respond to the PR discussions individually. I think all but 2 i'm happy to implement without further discussion.

@Gordonby
Copy link
Collaborator Author

Getting another strange CI failure @shenglol - any insight? It works fine locally.

An error occurred: Exception calling "Invoke" with "0" argument(s): "The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop:
Cannot find deleted vault 'akvtest3vbarm34t26ck' in location 'westus'"

@alex-frankel
Copy link

alex-frankel commented Apr 21, 2022

Thanks for all the feedback @bmoore-msft and iteration @Gordonby.

In general, I am aligned with Brian's thinking, though I had one thought that may split the difference:

Should we have a generic-deployment-script module that is the foundation for all these specific scenarios in which to use a deployment script. The generic-deployment-script would expose everything (or mostly everything) with good defaults, then the create-kv-certificate would use that module in its definition and not expose the same degree of parameters. Thoughts?

@shenglol
Copy link
Collaborator

Getting another strange CI failure @shenglol - any insight? It works fine locally.

An error occurred: Exception calling "Invoke" with "0" argument(s): "The running command stopped because the preference variable "ErrorActionPreference" or common parameter is set to Stop:
Cannot find deleted vault 'akvtest3vbarm34t26ck' in location 'westus'"

That's an error in cleaning up the resource group. It seems that deleting a key vault is an async operation inside the backend and we should wait some time before purging the key vault. I'm working on a fix now.

@shenglol
Copy link
Collaborator

Closing and reopening the PR to run the latest Deployment Test - CI.

@shenglol shenglol closed this Apr 21, 2022
@shenglol shenglol reopened this Apr 21, 2022
@bmoore-msft
Copy link
Collaborator

Thanks for all the feedback @bmoore-msft and iteration @Gordonby.

In general, I am aligned with Brian's thinking, though I had one thought that may split the difference:

Should we have a generic-deployment-script module that is the foundation for all these specific scenarios in which to use a deployment script. The generic-deployment-script would expose everything (or mostly everything) with good defaults, then the create-kv-certificate would use that module in its definition and not expose the same degree of parameters. Thoughts?

Do we have a generic deployment script module? If so, I do love the idea...

@alex-frankel
Copy link

Thanks for all the feedback @bmoore-msft

        Brian Moore (AZURE RESOURCE MANAGER)
        FTE and iteration @Gordonby
        
        Gordon Byers
        FTE.

In general, I am aligned with Brian's thinking, though I had one thought that may split the difference:
Should we have a generic-deployment-script module that is the foundation for all these specific scenarios in which to use a deployment script. The generic-deployment-script would expose everything (or mostly everything) with good defaults, then the create-kv-certificate would use that module in its definition and not expose the same degree of parameters. Thoughts?

Do we have a generic deployment script module? If so, I do love the idea...

We don't but we can make one, or @Gordonby or @MrMCake can have right of first refusal :)

@shenglol shenglol closed this Apr 21, 2022
@shenglol shenglol reopened this Apr 21, 2022
@Gordonby
Copy link
Collaborator Author

Gordonby commented Apr 22, 2022

We don't but we can make one, or @Gordonby or @MrMCake can have right of first refusal :)

There is a module for this resource in CARML. If it's suitable, then @MrMCake should probably PR.
If not, I can contribute one.

@bmoore-msft
Copy link
Collaborator

That's an error in cleaning up the resource group. It seems that deleting a key vault is an async operation inside the backend and we should wait some time before purging the key vault. I'm working on a fix now.

Why would we need to purge the vault?

@shenglol
Copy link
Collaborator

I'll need to update the MCR manifest before merging the PR since the module name is changed.

@shenglol shenglol added MCR Manifest Onboarded and removed Needs: Triage 🔍 Maintainers need to triage still labels Apr 23, 2022
Copy link
Collaborator

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

Thanks for adding the module!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Module Proposal]: Azure CLI DeploymentScript Module - AKV Certs in AGW
4 participants