Skip to content

Variant generator fixes + Azure provisioning support#344

Open
mkultraWasHere wants to merge 19 commits into
mainfrom
fix/variant-generator-bugs
Open

Variant generator fixes + Azure provisioning support#344
mkultraWasHere wants to merge 19 commits into
mainfrom
fix/variant-generator-bugs

Conversation

@mkultraWasHere

@mkultraWasHere mkultraWasHere commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes discovered during the an Azure deployment of a GOAD variant.

Added

  • dreadgoad env create now supports Azure (-p azure --region centralus) — scaffolds the full Terragrunt directory tree with auto-derived VNet, bastion, and controller subnet CIDRs
  • dreadgoad doctor checks for pypsrp and pysocks when provider is Azure
  • Azure bootstrap script pre-installs NuGet provider to avoid PackageManagement.dll corruption on fresh Marketplace VMs

Changed

  • Azure inventory templates for all 6 labs (GOAD, GOAD-Light, GOAD-Mini, NHA, DRACARYS, SCCM) now include full Ansible group definitions ([dc], [domain], [server], [trust], [adcs], etc.) and required [all:vars]
  • admin_user in Azure inventory templates changed from goadmin to administrator to match what the bootstrap script configures
  • Inventory bootstrapping (bootstrapFromProviderTemplate) resolves the lab directory from the active variant target instead of hardcoding GOAD
  • dreadgoad doctor ansible-core >=2.19 check now warns instead of failing when provider is Azure (the breakage is SSM-specific)
  • Azure network Terraform module auto-derives private_subnet_cidr and public_subnet_cidr from vnet_cidr via cidrsubnet() when not explicitly set
  • env list scans the correct infra directory per provider

Fixed

  • Variant-generated passwords no longer contain {, }, [, ] — these produced Jinja2 template sequences ({#, {{, {%) that crashed Ansible playbooks
  • Variant generator no longer produces "Robert" or "Brandon" as first names (collide with existing GOAD entity names)
  • Group/surname mapping conflicts no longer cause incorrect replacements when a group name matches a surname
  • Underscore-delimited keys (e.g. GenericWrite_missandei_viserys) are now transformed correctly using word-boundary matching
  • isTextFile no longer misclassifies extensionless files as binary
  • Replacement ordering no longer allows shorter names to shadow longer compound names

mkultraWasHere and others added 10 commits July 8, 2026 16:40
@mkultraWasHere @claude
fix(variant): fix 6 bugs in variant generator transform and mapping l…
fbc3aaf 
…ogic

Extensionless files (e.g., inventory_disable_vagrant) were silently
copied without text replacement. File paths containing entity names
were not renamed on disk, causing deployment failures. Group names
in compound strings (e.g., StarkWallpaper) were missed due to
overly aggressive word-boundary matching. Firstname collisions in
the Misc map caused silent overwrites and stale mapping.json entries.
Single-name users (drogon, missandei, hodor) were incorrectly mapped
to firstname.lastname format. Underscore-delimited ACL keys in
overlay files were not transformed because Go's \b treats _ as a
word character.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mkultraWasHere @claude
fix(variant): reconcile group/surname mapping conflicts and optimize …
7e96006 
…isTextFile

When a group name collides with a capitalized surname (e.g., "Targaryen"
is both an AD group and a surname from daenerys.targaryen), the Misc
entry is now removed in favor of the Groups mapping. This prevents
mapping.json from showing a stale surname-derived value while the
config uses the group-derived value. Also changed isTextFile to read
only the first 8 KiB instead of the entire file.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mkultraWasHere @claude
fix(variant): scope group/Misc reconciliation to name components only
cd28ab2 
The previous reconciliation removed any Misc entry matching a group
name, which could incorrectly delete hostname or city mappings if
they happened to collide. Now only removes entries tracked as name
components (firstname/surname), preserving other Misc entries.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mkultraWasHere @claude
fix(variant): remove Robert and Brandon from name generator word list
259cdaf 
These firstnames collide with the originalNames validation list
(from robert.baratheon and brandon.stark), causing false positive
validation warnings when the generator randomly picks them.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mkultraWasHere @claude
fix(variant): exclude Jinja-unsafe characters from generated passwords
7aa4513 
Remove []{}  from the password special character pool. These can produce
Jinja2 template sequences ({#, {{, {%}) that cause Ansible to crash
with "Missing end of comment tag" errors when passwords are embedded
in playbook templates.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix(azure): complete inventory templates and variant-aware bootstrapping
e24dabf 
Inventory bootstrapping hardcoded labName to "GOAD", ignoring active
variants. Now resolves to the variant target directory (e.g.
GOAD-dreadindex) when a variant is configured.

Azure inventory templates were missing all Ansible group definitions
([dc], [domain], [server], [trust], [adcs], etc.) and required
[all:vars] (domain_name, force_dns_server, dns_server_forwarder, etc.).
Without these groups, playbooks silently skipped tasks or failed with
undefined variable errors.

Also fixes admin_user from "goadmin" to "administrator" — the Azure
bootstrap script renames the SID-500 account to "administrator".

Updated templates for all 6 labs: GOAD, GOAD-Light, GOAD-Mini, NHA,
DRACARYS, SCCM.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix(doctor): add pypsrp check for Azure and make ansible-core version…
dd45b2b 
… provider-aware

Azure provisioning via Bastion+controller uses WinRM/PSRP, which
requires the pypsrp Python package. Add the existing pypsrp check to
runAzureChecks() so users get a clear doctor message instead of a
confusing Ansible error.

The ansible-core >=2.19 version check now warns instead of failing
when provider is Azure. The 2.19 breakage is SSM-specific (AWS) and
does not affect Azure WinRM/PSRP connections.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix(azure): auto-derive subnet CIDRs and pre-install NuGet in bootstrap
0bf9659 
The Azure network module had hardcoded subnet defaults (10.8.1.0/24)
that didn't adjust when vnet_cidr was changed, causing "subnet range
outside VNet" errors. Now uses cidrsubnet() to auto-derive subnets
from vnet_cidr, matching the AWS module pattern. Explicit overrides
still work.

Fresh Azure Marketplace VMs sometimes ship with a corrupted
PackageManagement.dll that causes "Catastrophic failure" when Ansible
tries to install PowerShell modules. Pre-installing the NuGet provider
in the bootstrap script avoids this.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
feat(azure): add Azure support to dreadgoad env create
ad0b750 
`env create` was AWS-only. Now supports Azure with provider-aware
scaffolding:

- Azure env.hcl with vnet_cidr, bastion/controller subnet CIDRs
  (auto-derived from VNet CIDR)
- Azure region.hcl with location instead of aws_region
- Defaults --reference to "test" for Azure (the existing Azure env)
- Azure inventory generation (resets IPs to PENDING)
- env list scans the correct infra directory per provider

Usage: dreadgoad env create alpha -p azure --region centralus

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
chore: add doc comments and use specialChars constant in seedRequired
278219b 
Add missing Go doc comments on createAzureEnvHCL, createAzureRegionHCL,
and generateAzureInventory. Replace hardcoded special character string
in seedRequired() with the specialChars constant to keep a single
source of truth.

Co-Authored-By: Claude <noreply@anthropic.com>
@dreadnode-renovate-bot dreadnode-renovate-bot Bot added lab/NHA Changes made to NHA lab lab/GOAD Changes made to GOAD lab lab/GOAD-Mini Changes made to GOAD-Mini lab area/ad-labs Changes made to AD lab definitions lab/SCCM Changes made to SCCM lab lab/GOAD-Light Changes made to GOAD-Light lab labels Jul 10, 2026
@mkultraWasHere mkultraWasHere requested a review from Copilot July 10, 2026 18:13
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR improves GOAD variant generation robustness and expands the CLI + infra scaffolding to better support Azure deployments (env creation, inventory templating, and Azure-specific preflight/bootstrap behavior).

Changes:

  • Adds Azure-aware environment scaffolding (env create) including Azure-specific env.hcl/region.hcl generation and inventory bootstrapping.
  • Fixes multiple variant-generator edge cases (password character set, word-boundary replacement for underscore-delimited keys, file renaming, extensionless-text detection) with added tests.
  • Updates Azure Terraform networking to auto-derive subnet CIDRs from vnet_cidr, and expands Azure inventory templates with full group definitions + updated admin_user.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
modules/terraform-azure-net/variables.tf Makes Azure subnet CIDRs optional so they can be auto-derived from vnet_cidr.
modules/terraform-azure-net/main.tf Derives public/private subnet CIDRs via cidrsubnet() when not explicitly set.
infra/azure/goad-deployment/test/centralus/goad/templates/bootstrap.ps1.tpl Pre-installs NuGet provider to reduce bootstrap failures on some Marketplace images.
cli/internal/variant/namegen.go Removes problematic first names and disallowed special characters from generated passwords.
cli/internal/variant/namegen_test.go Adds a regression test ensuring generated passwords avoid Jinja-breaking characters.
cli/internal/variant/generator.go Fixes replacement ordering/collisions, underscores-as-boundaries handling, extensionless-text handling, and path renaming.
cli/internal/variant/generator_test.go Adds focused regression tests covering edge cases fixed in the generator.
cli/internal/doctor/checks.go Adjusts Ansible version check behavior for Azure and adds Azure check for pypsrp/PySocks.
cli/internal/config/config.go Adds provider-aware infra base path resolution for Azure vs non-Azure layouts.
cli/cmd/provision.go Passes provider into the ansible-core version gate.
cli/cmd/env_cmd.go Adds Azure scaffolding helpers (subnet derivation, Azure env/region HCL, Azure inventory generation) and correct infra scanning per provider.
cli/cmd/env_cmd_test.go Adds unit tests for Azure subnet derivation.
ad/SCCM/providers/azure/inventory Expands Azure SCCM inventory with full groups and updated admin_user.
ad/NHA/providers/azure/inventory Expands Azure NHA inventory with full groups and updated admin_user.
ad/GOAD/providers/azure/inventory Expands Azure GOAD inventory with full groups and updated admin_user.
ad/GOAD-Mini/providers/azure/inventory Expands Azure GOAD-Mini inventory with full groups and updated admin_user.
ad/GOAD-Light/providers/azure/inventory Expands Azure GOAD-Light inventory with full groups and updated admin_user.
ad/DRACARYS/providers/azure/inventory Expands Azure DRACARYS inventory with full groups and updated admin_user.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cli/cmd/env_cmd.go
Comment thread cli/cmd/env_cmd.go Outdated
Comment thread cli/internal/doctor/checks.go
@mkultraWasHere @claude
fix: address PR review feedback from Copilot
0155b21 
- Guard against IPv6 CIDR in deriveAzureSubnets (To4() nil panic)
- Use ReplaceAllStringFunc instead of ReplaceAllString for envName
  replacement to avoid $ in env names corrupting regexp output
- Fix checkPyPSRP message from "SSH-mode" to "WinRM/PSRP" so it's
  accurate for both Azure and Ludus providers

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere mkultraWasHere requested a review from Copilot July 10, 2026 18:26
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comment thread cli/cmd/env_cmd.go
Comment thread modules/terraform-azure-net/variables.tf
mkultraWasHere and others added 2 commits July 10, 2026 14:56
@mkultraWasHere @claude
fix: address second round of PR review feedback
39a4b8f 
- generateAzureInventory falls back to the GOAD Azure provider
  inventory template when no reference inventory file exists (a fresh
  clone won't have test-inventory)
- Terraform private_subnet_cidr output now references local.private_subnet
  instead of var.private_subnet_cidr, which is empty by default after
  the auto-derive change

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix: use ReplaceAllStringFunc for AWS inventory generation too
97654f2 
Same regexp replacement vulnerability as the Azure path — envName and
region containing $ would corrupt output via capture group references.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere mkultraWasHere requested a review from Copilot July 10, 2026 19:11
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Comment thread cli/internal/variant/generator.go
@mkultraWasHere @claude
fix(variant): prevent dotless username collisions after truncation
c73182d 
When the original username has no dot (e.g. "missandei"), the generator
truncates "firstname.lastname" to just "firstname". Multiple dotless
users could get the same firstname, producing duplicate sAMAccountNames.
Now passes the truncated name through ensureUnique to add a counter
suffix if needed.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere mkultraWasHere requested a review from Copilot July 10, 2026 20:19
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

Comment thread cli/internal/variant/generator.go
@mkultraWasHere mkultraWasHere requested a review from Copilot July 10, 2026 21:00
Copilot AI reviewed Jul 10, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Comment thread cli/internal/variant/generator.go
Comment thread cli/internal/config/config.go
Comment thread modules/terraform-azure-net/variables.tf
@mkultraWasHere mkultraWasHere marked this pull request as ready for review July 10, 2026 21:44
mkultraWasHere and others added 5 commits July 10, 2026 18:42
@mkultraWasHere @claude
refactor: extract helpers in env scaffolding and variant name mapping
54abc98 
Break scaffoldEnv into focused functions (printEnvSummary, scaffoldHCL,
scaffoldLabConfig, scaffoldInventory, printNextSteps) for readability.
Extract setNameComponent in the variant generator to reduce duplication
in mapUserNameComponents. Update dreadgoad.yaml for Azure dreadindex
deployment.

Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
chore: regenerate terraform-azure-net docs after variable changes
e27ba4f 
Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix: wrap inventory error with context in scaffoldInventory
10acece 
Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
fix: discard unused err from f.Read in isTextFile (ineffassign)
8c4f938 
Co-Authored-By: Claude <noreply@anthropic.com>
@mkultraWasHere @claude
style: apply De Morgan's law to satisfy staticcheck QF1001
a89417e 
Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

area/ad-labs Changes made to AD lab definitions lab/GOAD Changes made to GOAD lab lab/GOAD-Light Changes made to GOAD-Light lab lab/GOAD-Mini Changes made to GOAD-Mini lab lab/NHA Changes made to NHA lab lab/SCCM Changes made to SCCM lab

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mkultraWasHere