From e292068a6bece2538552abd7317e6ffbe774ce6d Mon Sep 17 00:00:00 2001 From: Dylan Myers Date: Mon, 3 Aug 2026 16:04:47 -0400 Subject: [PATCH 1/2] fix(datagen): return errors instead of crashing on nil-domain developer errors (PIPE-1003) Assisted-by: Claude Opus 4.8 --- internal/datagen/datagen_errors_test.go | 74 +++++++++++++++++++++++++ internal/datagen/environment.go | 40 ++++++++++--- internal/datagen/environment_test.go | 56 +++++++++++++++---- internal/datagen/groups.go | 7 ++- internal/datagen/groups_test.go | 60 ++++++++++++++++---- internal/datagen/systems.go | 14 ++--- internal/datagen/systems_test.go | 62 ++++++++++++++------- internal/datagen/usernames.go | 16 ++++-- internal/datagen/usernames_test.go | 45 ++++++++++++--- 9 files changed, 298 insertions(+), 76 deletions(-) create mode 100644 internal/datagen/datagen_errors_test.go diff --git a/internal/datagen/datagen_errors_test.go b/internal/datagen/datagen_errors_test.go new file mode 100644 index 0000000..0497008 --- /dev/null +++ b/internal/datagen/datagen_errors_test.go @@ -0,0 +1,74 @@ +package datagen + +import ( + "errors" + "math/rand" + "testing" +) + +// These tests exercise the developer-error paths converted from panics to error +// returns (PIPE-1003). They double as regression guards: they assert that each +// constructor rejects a nil domain, and that GenerateEnvironment propagates a +// failure from any of its identity-generation stages. + +func TestGenerateUserIdentity_NilDomain(t *testing.T) { + u, err := GenerateUserIdentity(rand.New(rand.NewSource(1)), nil) + if err == nil || u != nil { + t.Errorf("GenerateUserIdentity(nil) = (%v, %v), want (nil, error)", u, err) + } +} + +func TestGenerateUsers_NilDomain(t *testing.T) { + users, err := GenerateUsers(1, 1, nil) + if err == nil || users != nil { + t.Errorf("GenerateUsers(nil) = (%v, %v), want (nil, error)", users, err) + } +} + +func TestGenerateGroups_NilDomain(t *testing.T) { + groups, err := GenerateGroups(1, 5, 0, nil, nil) + if err == nil || groups != nil { + t.Errorf("GenerateGroups(nil) = (%v, %v), want (nil, error)", groups, err) + } +} + +func TestGenerateSystems_NilDomainPropagates(t *testing.T) { + systems, err := generateSystems(1, 1, 1, 3, nil, GenerateDefaultNetworks()) + if err == nil || systems != nil { + t.Errorf("generateSystems(nil) = (%v, %v), want (nil, error)", systems, err) + } +} + +func TestGenerateEnvironment_PropagatesStageErrors(t *testing.T) { + boom := errors.New("boom") + seeds := &SeedConfig{Shared: 1} + + t.Run("users stage error", func(t *testing.T) { + orig := genUsers + genUsers = func(int64, int, *DomainIdentity) ([]*UserIdentity, error) { return nil, boom } + defer func() { genUsers = orig }() + if env, err := GenerateEnvironment(seeds, nil); !errors.Is(err, boom) || env != nil { + t.Errorf("want boom + nil env, got (%v, %v)", env, err) + } + }) + + t.Run("groups stage error", func(t *testing.T) { + orig := genGroups + genGroups = func(int64, int, int, *DomainIdentity, []*UserIdentity) ([]*GroupIdentity, error) { return nil, boom } + defer func() { genGroups = orig }() + if env, err := GenerateEnvironment(seeds, nil); !errors.Is(err, boom) || env != nil { + t.Errorf("want boom + nil env, got (%v, %v)", env, err) + } + }) + + t.Run("systems stage error", func(t *testing.T) { + orig := genSystems + genSystems = func(_, _, _ int64, _ int, _ *DomainIdentity, _ []*NetworkIdentity) ([]*SystemIdentity, error) { + return nil, boom + } + defer func() { genSystems = orig }() + if env, err := GenerateEnvironment(seeds, nil); !errors.Is(err, boom) || env != nil { + t.Errorf("want boom + nil env, got (%v, %v)", env, err) + } + }) +} diff --git a/internal/datagen/environment.go b/internal/datagen/environment.go index c6c27d1..d9f5791 100644 --- a/internal/datagen/environment.go +++ b/internal/datagen/environment.go @@ -85,6 +85,18 @@ type osRoleSpec struct { pool *Pool[string] } +// Stage seams. GenerateEnvironment calls its identity-generation stages +// through these package-level variables so tests can force a stage to fail and +// assert that GenerateEnvironment propagates the error. Production always uses +// the real implementations; the stages cannot fail today (GenerateEnvironment +// builds a valid domain), but the propagation is kept as a regression guard for +// when a stage gains a real failure mode. +var ( + genUsers = GenerateUsers + genGroups = GenerateGroups + genSystems = generateSystems +) + // GenerateEnvironment produces a fully cross-referenced environment from seeds and options. // // Per-identity-type seeds drive each independent stage of generation: @@ -95,7 +107,7 @@ type osRoleSpec struct { // IdentityApplications for applications attached to each system. Each stage // uses a fresh RNG seeded from its own field, so changing one seed only // re-randomizes that slice of the output. -func GenerateEnvironment(seeds *SeedConfig, opts *EnvironmentOpts) *Environment { +func GenerateEnvironment(seeds *SeedConfig, opts *EnvironmentOpts) (*Environment, error) { opts = defaultOpts(opts) // Generate domain @@ -108,11 +120,17 @@ func GenerateEnvironment(seeds *SeedConfig, opts *EnvironmentOpts) *Environment // Generate users userSeed := seeds.ResolveSeed(IdentityUsers) - users := GenerateUsers(userSeed, opts.UserCount, domain) + users, err := genUsers(userSeed, opts.UserCount, domain) + if err != nil { + return nil, err + } // Generate groups groupSeed := seeds.ResolveSeed(IdentityGroups) - groups := GenerateGroups(groupSeed, opts.GroupCount, opts.DomainAdminsCount, domain, users) + groups, err := genGroups(groupSeed, opts.GroupCount, opts.DomainAdminsCount, domain, users) + if err != nil { + return nil, err + } // Generate systems with a mix of OS/roles. Services and applications // have their own seeded RNGs so changing IdentityServices or @@ -120,7 +138,10 @@ func GenerateEnvironment(seeds *SeedConfig, opts *EnvironmentOpts) *Environment systemSeed := seeds.ResolveSeed(IdentitySystems) servicesSeed := seeds.ResolveSeed(IdentityServices) applicationsSeed := seeds.ResolveSeed(IdentityApplications) - systems := generateSystems(systemSeed, servicesSeed, applicationsSeed, opts.SystemCount, domain, networks) + systems, err := genSystems(systemSeed, servicesSeed, applicationsSeed, opts.SystemCount, domain, networks) + if err != nil { + return nil, err + } // Appliance identities (PIPE-1035): storage arrays and network hardware, // each with its own seed and a management interface bound to a subnet. @@ -138,7 +159,7 @@ func GenerateEnvironment(seeds *SeedConfig, opts *EnvironmentOpts) *Environment Systems: systems, StorageSystems: storageSystems, NetworkSystems: networkSystems, - } + }, nil } // managementNetwork returns the subnet to bind appliance management interfaces @@ -234,7 +255,7 @@ func generateNetworksList(seed int64, count int) []*NetworkIdentity { // resource specs). servicesSeed and applicationsSeed seed independent RNGs // for service and application generation so changes to either seed only // re-randomize that slice. -func generateSystems(systemSeed, servicesSeed, applicationsSeed int64, count int, domain *DomainIdentity, networks []*NetworkIdentity) []*SystemIdentity { +func generateSystems(systemSeed, servicesSeed, applicationsSeed int64, count int, domain *DomainIdentity, networks []*NetworkIdentity) ([]*SystemIdentity, error) { r := rand.New(rand.NewSource(systemSeed)) // #nosec G404 rServices := rand.New(rand.NewSource(servicesSeed)) // #nosec G404 rApplications := rand.New(rand.NewSource(applicationsSeed)) // #nosec G404 @@ -253,7 +274,10 @@ func generateSystems(systemSeed, servicesSeed, applicationsSeed int64, count int for i := 0; i < count; i++ { // Pick OS/role using weighted selection spec := weightedSelect(r, specs, weights) - sys := GenerateSystemIdentity(r, spec.os, spec.role, domain, spec.pool) + sys, err := GenerateSystemIdentity(r, spec.os, spec.role, domain, spec.pool) + if err != nil { + return nil, err + } // Services and applications use their own RNGs so the seeds in // SeedConfig actually drive what's generated, per identity type. @@ -277,7 +301,7 @@ func generateSystems(systemSeed, servicesSeed, applicationsSeed int64, count int systems[i] = sys } - return systems + return systems, nil } // weightedSelect picks an item using weighted random selection. diff --git a/internal/datagen/environment_test.go b/internal/datagen/environment_test.go index b6bf965..6ca796b 100644 --- a/internal/datagen/environment_test.go +++ b/internal/datagen/environment_test.go @@ -17,7 +17,10 @@ func TestGenerateEnvironment(t *testing.T) { NetworkCount: 4, } - env := GenerateEnvironment(seeds, opts) + env, err := GenerateEnvironment(seeds, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } t.Run("domain set", func(t *testing.T) { if env.Domain == nil { @@ -86,7 +89,10 @@ func TestGenerateEnvironment(t *testing.T) { } func TestGenerateEnvironmentComposesAppliances(t *testing.T) { - env := GenerateEnvironment(&SeedConfig{Shared: 42}, &EnvironmentOpts{StorageSystemCount: 3, NetworkSystemCount: 5}) + env, err := GenerateEnvironment(&SeedConfig{Shared: 42}, &EnvironmentOpts{StorageSystemCount: 3, NetworkSystemCount: 5}) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } if len(env.StorageSystems) != 3 { t.Fatalf("StorageSystems = %d, want 3", len(env.StorageSystems)) @@ -116,7 +122,11 @@ func TestGenerateEnvironmentDeterministicAppliances(t *testing.T) { mk := func() *Environment { s := NewSeedConfig() s.Shared = 7 - return GenerateEnvironment(s, &EnvironmentOpts{StorageSystemCount: 2, NetworkSystemCount: 2}) + env, err := GenerateEnvironment(s, &EnvironmentOpts{StorageSystemCount: 2, NetworkSystemCount: 2}) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } + return env } a, b := mk(), mk() if a.StorageSystems[0].Serial != b.StorageSystems[0].Serial { @@ -162,7 +172,10 @@ func TestBindManagementInterface(t *testing.T) { func TestGenerateEnvironmentDefaults(t *testing.T) { seeds := NewSeedConfig() seeds.Shared = 42 - env := GenerateEnvironment(seeds, nil) + env, err := GenerateEnvironment(seeds, nil) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } if env.Domain.Name != "blitz.local" { t.Errorf("default domain should be 'blitz.local', got %q", env.Domain.Name) @@ -190,8 +203,14 @@ func TestGenerateEnvironmentDeterministic(t *testing.T) { Now: now, } - env1 := GenerateEnvironment(seeds1, opts) - env2 := GenerateEnvironment(seeds2, opts) + env1, err := GenerateEnvironment(seeds1, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } + env2, err := GenerateEnvironment(seeds2, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } if env1.Domain.DomainSID != env2.Domain.DomainSID { t.Error("same seed should produce same DomainSID") @@ -227,7 +246,10 @@ func TestGenerateEnvironmentExtendsNetworks(t *testing.T) { NetworkCount: 10, Now: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), } - env := GenerateEnvironment(seeds, opts) + env, err := GenerateEnvironment(seeds, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } if len(env.Networks) != 10 { t.Errorf("expected 10 networks, got %d", len(env.Networks)) } @@ -250,12 +272,18 @@ func TestGenerateEnvironmentPerTypeSeedsIndependent(t *testing.T) { base.Shared = 42 opts := &EnvironmentOpts{SystemCount: 5, UserCount: 5, GroupCount: 5, NetworkCount: 4, Now: now} - envA := GenerateEnvironment(base, opts) + envA, err := GenerateEnvironment(base, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } altered := NewSeedConfig() altered.Shared = 42 altered.Services = 777 // override only services - envB := GenerateEnvironment(altered, opts) + envB, err := GenerateEnvironment(altered, opts) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } // Domain and hostnames must be identical — they don't depend on IdentityServices. if envA.Domain.DomainSID != envB.Domain.DomainSID { @@ -297,8 +325,14 @@ func TestGenerateEnvironmentCARelativeValidityWindow(t *testing.T) { seeds := NewSeedConfig() seeds.Shared = 42 - envA := GenerateEnvironment(seeds, &EnvironmentOpts{SystemCount: 1, UserCount: 1, GroupCount: 5}) - envB := GenerateEnvironment(seeds, &EnvironmentOpts{SystemCount: 1, UserCount: 1, GroupCount: 5}) + envA, err := GenerateEnvironment(seeds, &EnvironmentOpts{SystemCount: 1, UserCount: 1, GroupCount: 5}) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } + envB, err := GenerateEnvironment(seeds, &EnvironmentOpts{SystemCount: 1, UserCount: 1, GroupCount: 5}) + if err != nil { + t.Fatalf("GenerateEnvironment: %v", err) + } // Same relative span (both should be 10 years total, within tolerance). wantSpan := envA.Domain.CA.ValidTo.Sub(envA.Domain.CA.ValidFrom) diff --git a/internal/datagen/groups.go b/internal/datagen/groups.go index d06b5b3..ba5400f 100644 --- a/internal/datagen/groups.go +++ b/internal/datagen/groups.go @@ -112,7 +112,10 @@ func defaultDomainAdminsCount(userCount int) int { // User selection for Domain Admins is sampled without replacement (Fisher-Yates // partial shuffle), so no user appears twice in domainAdmins.MemberSIDs and // no user has duplicate Domain Admins references in their GroupSIDs. -func GenerateGroups(seed int64, targetTotal int, adminCount int, domain *DomainIdentity, users []*UserIdentity) []*GroupIdentity { +func GenerateGroups(seed int64, targetTotal int, adminCount int, domain *DomainIdentity, users []*UserIdentity) ([]*GroupIdentity, error) { + if domain == nil { + return nil, fmt.Errorf("datagen: GenerateGroups: domain must not be nil") + } r := rand.New(rand.NewSource(seed)) // #nosec G404 dcSuffix := domainToDC(domain.Name) @@ -152,7 +155,7 @@ func GenerateGroups(seed int64, targetTotal int, adminCount int, domain *DomainI // Assign users to groups assignUsersToGroups(r, groups, users, adminCount) - return groups + return groups, nil } // assignUsersToGroups distributes users across groups: every user joins diff --git a/internal/datagen/groups_test.go b/internal/datagen/groups_test.go index 267166a..4285d63 100644 --- a/internal/datagen/groups_test.go +++ b/internal/datagen/groups_test.go @@ -26,8 +26,14 @@ func TestGroupTypes(t *testing.T) { func TestGenerateGroups(t *testing.T) { domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 20, domain) - groups := GenerateGroups(42, 10, 0, domain, users) + users, err := GenerateUsers(42, 20, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } + groups, err := GenerateGroups(42, 10, 0, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } t.Run("correct count", func(t *testing.T) { if len(groups) < 10 { @@ -89,8 +95,14 @@ func TestGenerateGroups(t *testing.T) { }) t.Run("deterministic", func(t *testing.T) { - g1 := GenerateGroups(99, 5, 0, domain, users) - g2 := GenerateGroups(99, 5, 0, domain, users) + g1, err := GenerateGroups(99, 5, 0, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } + g2, err := GenerateGroups(99, 5, 0, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } for i := range g1 { if g1[i].Name != g2[i].Name { t.Errorf("same seed should produce same groups: %q vs %q", g1[i].Name, g2[i].Name) @@ -103,8 +115,14 @@ func TestGenerateGroupsBuiltinFloor(t *testing.T) { // Built-in AD groups (n=9) are always included; targetTotal below the floor // returns just the built-ins, not a truncated subset. domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 5, domain) - groups := GenerateGroups(42, 3, 0, domain, users) + users, err := GenerateUsers(42, 5, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } + groups, err := GenerateGroups(42, 3, 0, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } if len(groups) != len(builtinGroups) { t.Errorf("targetTotal=3 with %d built-ins should return %d groups, got %d", len(builtinGroups), len(builtinGroups), len(groups)) @@ -114,8 +132,14 @@ func TestGenerateGroupsBuiltinFloor(t *testing.T) { func TestGenerateGroupsCap(t *testing.T) { // targetTotal beyond MaxGroupCount caps at MaxGroupCount. domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 5, domain) - groups := GenerateGroups(42, 100, 0, domain, users) + users, err := GenerateUsers(42, 5, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } + groups, err := GenerateGroups(42, 100, 0, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } if len(groups) != MaxGroupCount { t.Errorf("targetTotal=100 should return MaxGroupCount=%d groups, got %d", MaxGroupCount, len(groups)) } @@ -145,9 +169,15 @@ func TestDomainAdminsUniqueAndExact(t *testing.T) { // Explicit adminCount: exactly that many unique users; no duplicates in // either MemberSIDs or any user's GroupSIDs. domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 100, domain) + users, err := GenerateUsers(42, 100, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } const want = 10 - groups := GenerateGroups(42, 12, want, domain, users) + groups, err := GenerateGroups(42, 12, want, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } var adminGroup *GroupIdentity for _, g := range groups { @@ -188,8 +218,14 @@ func TestDomainAdminsCappedToUserCount(t *testing.T) { // adminCount > len(users) caps at len(users) instead of looping forever // or emitting duplicates. domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 3, domain) - groups := GenerateGroups(42, 9, 100, domain, users) + users, err := GenerateUsers(42, 3, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } + groups, err := GenerateGroups(42, 9, 100, domain, users) + if err != nil { + t.Fatalf("GenerateGroups: %v", err) + } for _, g := range groups { if g.Name == "Domain Admins" { if len(g.MemberSIDs) != len(users) { diff --git a/internal/datagen/systems.go b/internal/datagen/systems.go index 2fc3f4d..05089dd 100644 --- a/internal/datagen/systems.go +++ b/internal/datagen/systems.go @@ -112,16 +112,14 @@ type CertInfo struct { // // domain must be non-nil and have a populated CA — those fields are used // unconditionally to construct the system's FQDN, OU path, and TLS cert. -// Passing a nil domain or a domain with a nil CA panics with a clear -// message; this is a developer-error class of failure (the same input -// would have been caught at the first test run). PIPE-1003 tracks -// converting these panics to error returns ahead of the embed seam split. -func GenerateSystemIdentity(r *rand.Rand, os OSType, role SystemRole, domain *DomainIdentity, names *Pool[string]) *SystemIdentity { +// Passing a nil domain or a domain with a nil CA returns an error rather than +// panicking, so an embedding host handles bad input without crashing. +func GenerateSystemIdentity(r *rand.Rand, os OSType, role SystemRole, domain *DomainIdentity, names *Pool[string]) (*SystemIdentity, error) { if domain == nil { - panic("datagen: GenerateSystemIdentity: domain must not be nil") + return nil, fmt.Errorf("datagen: GenerateSystemIdentity: domain must not be nil") } if domain.CA == nil { - panic("datagen: GenerateSystemIdentity: domain.CA must not be nil") + return nil, fmt.Errorf("datagen: GenerateSystemIdentity: domain.CA must not be nil") } // Pick hostname style based on OS @@ -180,7 +178,7 @@ func GenerateSystemIdentity(r *rand.Rand, os OSType, role SystemRole, domain *Do MemoryMB: mem, DiskGB: disk, Cert: cert, - } + }, nil } // generateResourceSpecs returns CPU, memory, disk based on role. diff --git a/internal/datagen/systems_test.go b/internal/datagen/systems_test.go index 6f8778c..4a6d851 100644 --- a/internal/datagen/systems_test.go +++ b/internal/datagen/systems_test.go @@ -51,7 +51,10 @@ func TestGenerateSystemIdentity(t *testing.T) { domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) t.Run("linux server", func(t *testing.T) { - sys := GenerateSystemIdentity(r, OSLinux, RoleServer, domain, NorseNames) + sys, err := GenerateSystemIdentity(r, OSLinux, RoleServer, domain, NorseNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if sys.OS != OSLinux { t.Errorf("expected OS %q, got %q", OSLinux, sys.OS) } @@ -73,7 +76,10 @@ func TestGenerateSystemIdentity(t *testing.T) { }) t.Run("windows workstation", func(t *testing.T) { - sys := GenerateSystemIdentity(r, OSWindows, RoleWorkstation, domain, RomanNames) + sys, err := GenerateSystemIdentity(r, OSWindows, RoleWorkstation, domain, RomanNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if sys.OS != OSWindows { t.Errorf("expected OS %q, got %q", OSWindows, sys.OS) } @@ -83,7 +89,10 @@ func TestGenerateSystemIdentity(t *testing.T) { }) t.Run("domain controller", func(t *testing.T) { - sys := GenerateSystemIdentity(r, OSWindows, RoleDC, domain, GreekNames) + sys, err := GenerateSystemIdentity(r, OSWindows, RoleDC, domain, GreekNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if sys.Role != RoleDC { t.Errorf("expected Role %q, got %q", RoleDC, sys.Role) } @@ -93,7 +102,10 @@ func TestGenerateSystemIdentity(t *testing.T) { }) t.Run("has cert", func(t *testing.T) { - sys := GenerateSystemIdentity(r, OSLinux, RoleServer, domain, NorseNames) + sys, err := GenerateSystemIdentity(r, OSLinux, RoleServer, domain, NorseNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if sys.Cert == nil { t.Fatal("expected system to have a cert") } @@ -106,7 +118,10 @@ func TestGenerateSystemIdentity(t *testing.T) { }) t.Run("has OU path", func(t *testing.T) { - sys := GenerateSystemIdentity(r, OSWindows, RoleServer, domain, RomanNames) + sys, err := GenerateSystemIdentity(r, OSWindows, RoleServer, domain, RomanNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if !strings.HasPrefix(sys.OUPath, "OU=") { t.Errorf("OUPath %q should start with 'OU='", sys.OUPath) } @@ -116,27 +131,29 @@ func TestGenerateSystemIdentity(t *testing.T) { }) } -func TestGenerateSystemIdentityPanicsOnNilInputs(t *testing.T) { +func TestGenerateSystemIdentityErrorsOnNilInputs(t *testing.T) { r := rand.New(rand.NewSource(1)) domain := GenerateDomainIdentity(1, "", time.Now()) - t.Run("nil domain panics", func(t *testing.T) { - defer func() { - if recover() == nil { - t.Error("expected panic on nil domain, got none") - } - }() - GenerateSystemIdentity(r, OSLinux, RoleServer, nil, NorseNames) + t.Run("nil domain errors", func(t *testing.T) { + sys, err := GenerateSystemIdentity(r, OSLinux, RoleServer, nil, NorseNames) + if err == nil { + t.Error("expected error on nil domain, got none") + } + if sys != nil { + t.Errorf("expected nil identity on error, got %v", sys) + } }) - t.Run("domain with nil CA panics", func(t *testing.T) { - defer func() { - if recover() == nil { - t.Error("expected panic on nil domain.CA, got none") - } - }() + t.Run("domain with nil CA errors", func(t *testing.T) { brokenDomain := &DomainIdentity{Name: domain.Name, DomainSID: domain.DomainSID, CA: nil} - GenerateSystemIdentity(r, OSLinux, RoleServer, brokenDomain, NorseNames) + sys, err := GenerateSystemIdentity(r, OSLinux, RoleServer, brokenDomain, NorseNames) + if err == nil { + t.Error("expected error on nil domain.CA, got none") + } + if sys != nil { + t.Errorf("expected nil identity on error, got %v", sys) + } }) } @@ -157,7 +174,10 @@ func TestSystemResourceRanges(t *testing.T) { for role, spec := range specs { for i := 0; i < 50; i++ { - sys := GenerateSystemIdentity(r, OSLinux, role, domain, NorseNames) + sys, err := GenerateSystemIdentity(r, OSLinux, role, domain, NorseNames) + if err != nil { + t.Fatalf("GenerateSystemIdentity: %v", err) + } if sys.CPUCores < spec.minCPU || sys.CPUCores > spec.maxCPU { t.Errorf("role %s: CPUCores %d out of [%d,%d]", role, sys.CPUCores, spec.minCPU, spec.maxCPU) } diff --git a/internal/datagen/usernames.go b/internal/datagen/usernames.go index ad0f058..b9ab631 100644 --- a/internal/datagen/usernames.go +++ b/internal/datagen/usernames.go @@ -63,7 +63,10 @@ type UserIdentity struct { } // GenerateUserIdentity creates a random user identity within the given domain. -func GenerateUserIdentity(r *rand.Rand, domain *DomainIdentity) *UserIdentity { +func GenerateUserIdentity(r *rand.Rand, domain *DomainIdentity) (*UserIdentity, error) { + if domain == nil { + return nil, fmt.Errorf("datagen: GenerateUserIdentity: domain must not be nil") + } first := FirstNames.Random(r) last := Surnames.Random(r) @@ -98,7 +101,7 @@ func GenerateUserIdentity(r *rand.Rand, domain *DomainIdentity) *UserIdentity { Department: dept, Title: title, DN: dn, - } + }, nil } // GenerateUsers produces a deterministic set of users from a seed. @@ -109,19 +112,22 @@ func GenerateUserIdentity(r *rand.Rand, domain *DomainIdentity) *UserIdentity { // the CN component of DN — so the returned UserIdentity stays internally // consistent. In real AD, sAMAccountName, UPN, and mail must all be unique; // the suffix scheme mirrors that. -func GenerateUsers(seed int64, count int, domain *DomainIdentity) []*UserIdentity { +func GenerateUsers(seed int64, count int, domain *DomainIdentity) ([]*UserIdentity, error) { r := rand.New(rand.NewSource(seed)) // #nosec G404 users := make([]*UserIdentity, count) seen := make(map[string]int) for i := range users { - u := GenerateUserIdentity(r, domain) + u, err := GenerateUserIdentity(r, domain) + if err != nil { + return nil, err + } seen[u.Username]++ if seen[u.Username] > 1 { disambiguateUser(u, seen[u.Username]) } users[i] = u } - return users + return users, nil } // disambiguateUser appends a numeric suffix to all identifier-bearing fields diff --git a/internal/datagen/usernames_test.go b/internal/datagen/usernames_test.go index 5d9a39b..16bbdcb 100644 --- a/internal/datagen/usernames_test.go +++ b/internal/datagen/usernames_test.go @@ -33,7 +33,10 @@ func TestGenerateUserIdentity(t *testing.T) { domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) t.Run("basic fields populated", func(t *testing.T) { - user := GenerateUserIdentity(r, domain) + user, err := GenerateUserIdentity(r, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } if user.FirstName == "" { t.Error("FirstName should not be empty") } @@ -52,7 +55,10 @@ func TestGenerateUserIdentity(t *testing.T) { }) t.Run("display name is title case", func(t *testing.T) { - user := GenerateUserIdentity(r, domain) + user, err := GenerateUserIdentity(r, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } parts := strings.Split(user.DisplayName, " ") if len(parts) != 2 { t.Errorf("DisplayName %q should be 'First Last'", user.DisplayName) @@ -60,14 +66,20 @@ func TestGenerateUserIdentity(t *testing.T) { }) t.Run("SID format", func(t *testing.T) { - user := GenerateUserIdentity(r, domain) + user, err := GenerateUserIdentity(r, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } if !strings.HasPrefix(user.SID, domain.DomainSID+"-") { t.Errorf("user SID %q should start with domain SID %q", user.SID, domain.DomainSID) } }) t.Run("has department and title", func(t *testing.T) { - user := GenerateUserIdentity(r, domain) + user, err := GenerateUserIdentity(r, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } if user.Department == "" { t.Error("Department should not be empty") } @@ -77,7 +89,10 @@ func TestGenerateUserIdentity(t *testing.T) { }) t.Run("DN format", func(t *testing.T) { - user := GenerateUserIdentity(r, domain) + user, err := GenerateUserIdentity(r, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } if !strings.HasPrefix(user.DN, "CN=") { t.Errorf("DN %q should start with 'CN='", user.DN) } @@ -89,8 +104,14 @@ func TestGenerateUserIdentity(t *testing.T) { t.Run("deterministic", func(t *testing.T) { r1 := rand.New(rand.NewSource(99)) r2 := rand.New(rand.NewSource(99)) - u1 := GenerateUserIdentity(r1, domain) - u2 := GenerateUserIdentity(r2, domain) + u1, err := GenerateUserIdentity(r1, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } + u2, err := GenerateUserIdentity(r2, domain) + if err != nil { + t.Fatalf("GenerateUserIdentity: %v", err) + } if u1.Username != u2.Username { t.Errorf("same seed should produce same username: %q vs %q", u1.Username, u2.Username) } @@ -99,7 +120,10 @@ func TestGenerateUserIdentity(t *testing.T) { func TestGenerateUsers(t *testing.T) { domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 20, domain) + users, err := GenerateUsers(42, 20, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } if len(users) != 20 { t.Errorf("expected 20 users, got %d", len(users)) } @@ -121,7 +145,10 @@ func TestGenerateUsersInternalConsistency(t *testing.T) { // suffix on Username must show up in UPN/Email local-parts, and // DisplayName must match the CN portion of DN. domain := GenerateDomainIdentity(42, "contoso.com", time.Now()) - users := GenerateUsers(42, 200, domain) + users, err := GenerateUsers(42, 200, domain) + if err != nil { + t.Fatalf("GenerateUsers: %v", err) + } upns := make(map[string]bool) emails := make(map[string]bool) From 633eac200c1f6effa7a3156bf6a64d45833b471e Mon Sep 17 00:00:00 2001 From: Dylan Myers Date: Mon, 3 Aug 2026 16:07:08 -0400 Subject: [PATCH 2/2] refactor(datagen): add error-returning parseCIDRs primitive; keep mustParseCIDRs for constants (PIPE-1003) Assisted-by: Claude Opus 4.8 --- internal/datagen/networks.go | 25 +++++++++++++++++++------ internal/datagen/networks_parse_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 internal/datagen/networks_parse_test.go diff --git a/internal/datagen/networks.go b/internal/datagen/networks.go index 49ea2df..79e2a17 100644 --- a/internal/datagen/networks.go +++ b/internal/datagen/networks.go @@ -236,19 +236,32 @@ func RandomIPInCIDRv6(r *rand.Rand, cidr string) string { return result.String() } -// mustParseCIDRs parses the given CIDR strings at package init time. Bad -// input here means a typo in a hardcoded literal in this file, so panicking -// is the right failure mode — it surfaces immediately on the first test run. -func mustParseCIDRs(cidrs ...string) []*net.IPNet { +// parseCIDRs parses CIDR strings into *net.IPNet, returning an error on the +// first bad entry. Use this for any runtime- or host-supplied CIDRs so a bad +// value surfaces as an error rather than a crash. +func parseCIDRs(cidrs ...string) ([]*net.IPNet, error) { out := make([]*net.IPNet, 0, len(cidrs)) for _, c := range cidrs { _, ipNet, err := net.ParseCIDR(c) if err != nil { - panic(fmt.Sprintf("datagen: invalid CIDR literal %q: %v", c, err)) + return nil, fmt.Errorf("invalid CIDR %q: %w", c, err) } out = append(out, ipNet) } - return out + return out, nil +} + +// mustParseCIDRs parses the hardcoded, compile-time-constant CIDR literals in +// this file's reserved-block tables. A bad literal is a programming error, so +// it panics — the same fail-fast idiom as regexp.MustCompile, caught by tests +// rather than by user input. Runtime- or host-supplied CIDRs must go through +// parseCIDRs (or ValidateCIDR / ValidateIPv6CIDR) instead. +func mustParseCIDRs(cidrs ...string) []*net.IPNet { + nets, err := parseCIDRs(cidrs...) + if err != nil { + panic(fmt.Sprintf("datagen: %v", err)) + } + return nets } // isReservedIPv4 reports whether the given IPv4 address falls in any block diff --git a/internal/datagen/networks_parse_test.go b/internal/datagen/networks_parse_test.go new file mode 100644 index 0000000..ca96bae --- /dev/null +++ b/internal/datagen/networks_parse_test.go @@ -0,0 +1,25 @@ +package datagen + +import "testing" + +func TestParseCIDRs(t *testing.T) { + nets, err := parseCIDRs("10.0.0.0/8", "2001:db8::/32") + if err != nil { + t.Fatalf("parseCIDRs(valid) error = %v, want nil", err) + } + if len(nets) != 2 { + t.Errorf("parseCIDRs(valid) returned %d nets, want 2", len(nets)) + } + if _, err := parseCIDRs("10.0.0.0/8", "garbage"); err == nil { + t.Error("parseCIDRs with a bad entry should return an error") + } +} + +func TestMustParseCIDRs_PanicsOnBadLiteral(t *testing.T) { + defer func() { + if recover() == nil { + t.Error("mustParseCIDRs should panic on a malformed literal") + } + }() + mustParseCIDRs("not-a-cidr") +}