Skip to content
This repository was archived by the owner on Sep 26, 2024. It is now read-only.

Robe/testing ad #74

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Robe/testing ad #74

wants to merge 15 commits into from

Conversation

zentron
Copy link

@zentron zentron commented Mar 29, 2022

No description provided.

required_providers {
azurerm = {
source = "hashicorp/azurerm"
version = "~>2.0"

Choose a reason for hiding this comment

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

You should consider jumping up to v3 which released this week, it might break some of your file below but best to start from current, I think.

Comment on lines +7 to +8
#We ned to refresh the state since the public vm IP isn't allocated when the resource is first provisioned
terraform refresh

Choose a reason for hiding this comment

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

That's a surprise, skimming your main.tf it looks like the output should contain the public IP as soon as apply is done.

Copy link
Author

Choose a reason for hiding this comment

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

I know right. Apparently it should be set but there seems to be an issue that sounds like its pretty common with Dynamic IPs in Azure. I would have thought but setting the depends in the output that it would get it once the Vm becomes available but apparently not

protocol = "Tcp"
source_port_range = "*"
destination_port_range = "389"
source_address_prefix = "*"

Choose a reason for hiding this comment

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

If this is intended to be used ephemerally, consider using http://ipv4.icanhazip.com or similar to get the IP of the environment running this infra up and defaulting the inbound security rule here to only allow that IP.

Copy link
Author

Choose a reason for hiding this comment

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

right thats an interesting idea. Looks like it wouldnt be too complex either

data "http" "myip" {
  url = "http://ipv4.icanhazip.com"
}

location = azurerm_resource_group.rg.location
size = "Standard_F2"
admin_username = "adminuser"
admin_password = random_password.password.result

Choose a reason for hiding this comment

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

Nice!

Comment on lines +108 to +113
source_image_reference {
publisher = "MicrosoftWindowsServer"
offer = "WindowsServer"
sku = "2016-Datacenter"
version = "latest"
}

Choose a reason for hiding this comment

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

Consider baking the AD setup into a packer script and reference that image output here. VM extensions can be pretty brutal for setting this sort of thing up; despite your use of depends_on I don't know if Az will guarantee that they run in order or necessarily succeed before returning control back to Terraform. If you aren't able to use a golden image, consider using Ansible or similar to set up the VM once terraform's provisioned it, that should be more reliable.

Copy link
Author

Choose a reason for hiding this comment

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

This does tend to work, the only complexity is that the AD Service install requires a reboot which then causes the configuration scripts not to run through the CustomScriptExtension resource.
Hence the RunCommandWindows that comes afterwards.

Choose a reason for hiding this comment

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

It may well have worked for you a small number of times, but depending on how frequent this is run, the tolerance for failure may be quite low. Generally for this sort of thing I'd recommend baking as much setup as possible into a VM image.

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

Successfully merging this pull request may close these issues.

2 participants