Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion test/e2e/v2/cmd/create-guests/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const defaultNamespace = "clusters"

// envConfig captures the common environment configuration.
type envConfig struct {
testPlanPath string
prowJobID string
sharedDir string
artifactDir string
Expand Down Expand Up @@ -104,6 +105,7 @@ func loadEnvConfig() envConfig {
}

cfg := envConfig{
testPlanPath: os.Getenv("TEST_PLAN"),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
prowJobID: mustGetenv("PROW_JOB_ID"),
sharedDir: sharedDir,
artifactDir: mustGetenv("ARTIFACT_DIR"),
Expand All @@ -130,7 +132,17 @@ func loadEnvConfig() envConfig {
}

func run(ctx context.Context, cfg envConfig) error {
specs := cfg.platform.ClusterSpecs(cfg.releaseImage, cfg.n1Image)
plan, err := lifecycle.ResolveTestPlan(cfg.testPlanPath, cfg.platform)
if err != nil {
return fmt.Errorf("resolving test plan: %w", err)
}
log.Printf("Using test plan %q", plan.Name)

allSpecs := cfg.platform.ClusterSpecs(cfg.releaseImage, cfg.n1Image)
if err := plan.Validate(allSpecs); err != nil {
return err
}
specs := plan.FilterClusterSpecs(allSpecs)

// Phase 0: The manifest must exist before any infra is provisioned so
// destroy-guests can always clean up, even if create-guests fails
Expand Down
29 changes: 20 additions & 9 deletions test/e2e/v2/cmd/run-tests/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,17 @@ func main() {

sharedDir := requireEnv("SHARED_DIR")
artifactDir := requireEnv("ARTIFACT_DIR")
releaseImage := os.Getenv("RELEASE_IMAGE_LATEST")

eventuallyVerbose := os.Getenv("EVENTUALLY_VERBOSE")
if eventuallyVerbose == "" {
eventuallyVerbose = defaultVerbose
}
os.Setenv("EVENTUALLY_VERBOSE", eventuallyVerbose)

if v := os.Getenv("RELEASE_IMAGE_LATEST"); v != "" {
os.Setenv("E2E_LATEST_RELEASE_IMAGE", v)
}

manifest, err := lifecycle.ReadManifest(sharedDir)
if err != nil {
log.Fatalf("Failed to read cluster manifest: %v", err)
Expand All @@ -63,9 +66,18 @@ func main() {
// Let the platform set up any env vars it needs for tests.
platform.SetupTestEnv(sharedDir)

matrix := platform.TestMatrix(releaseImage)
testPlanPath := os.Getenv("TEST_PLAN")
plan, err := lifecycle.ResolveTestPlan(testPlanPath, platform)
if err != nil {
log.Fatalf("Failed to resolve test plan: %v", err)
}
log.Printf("Using test plan %q", plan.Name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

run-tests resolves the plan here but never calls plan.Validate() — only ResolveVariants runs, which checks variants against the manifest but not group names or path safety. So the path-separator guard and the new duplicate-name check in TestMatrix.Validate() only actually run in create-guests, not in the binary that writes the JUnit files. A malformed TEST_PLAN (unsafe or duplicate Name) therefore panics inside the per-group worker goroutines (g.JUnitFile() at lines 96/119, no recover), tearing down the whole run mid-flight instead of failing fast. Calling plan.Validate() right after ResolveTestPlan would make both checks effective here and turn this into a clean up-front error. (The CI pipeline is mostly shielded today since create-guests validates first, but a standalone run-tests isn't.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cleaned this all up so that validation covers the path checks and makes the panic a backstop (i.e. if you panic it's because you forgot to call validate, which is explicitly required / a bug)


if err := plan.TestMatrix.Validate(); err != nil {
log.Fatalf("Invalid test plan: %v", err)
}

clustersByVariant, err := matrix.ResolveVariants(manifest)
clustersByVariant, err := plan.TestMatrix.ResolveVariants(manifest)
if err != nil {
log.Fatalf("Manifest/test matrix mismatch: %v", err)
}
Expand All @@ -77,15 +89,15 @@ func main() {
)

// Launch parallel test groups.
for _, g := range matrix.Parallel {
for _, g := range plan.TestMatrix.Parallel {
g := g
entry := clustersByVariant[g.Variant]
wg.Add(1)
go func() {
defer wg.Done()
log.Printf("Running %s tests against %s...", g.Name, entry.Name)
err := runTestBinary(testBinary, entry.Name, entry.Namespace, g.LabelFilter, g.Skip,
filepath.Join(artifactDir, g.JUnitFile), g.ExtraEnv)
filepath.Join(artifactDir, g.JUnitFile()))
mu.Lock()
results = append(results, testResult{name: g.Name, err: err})
mu.Unlock()
Expand All @@ -99,7 +111,7 @@ func main() {

// Launch sequential groups (each group runs in its own goroutine,
// but steps within a group run one after another).
for _, sg := range matrix.Sequential {
for _, sg := range plan.TestMatrix.Sequential {
sg := sg
wg.Add(1)
go func() {
Expand All @@ -108,7 +120,7 @@ func main() {
entry := clustersByVariant[step.Variant]
log.Printf("Running %s tests against %s...", step.Name, entry.Name)
err := runTestBinary(testBinary, entry.Name, entry.Namespace, step.LabelFilter, step.Skip,
filepath.Join(artifactDir, step.JUnitFile), step.ExtraEnv)
filepath.Join(artifactDir, step.JUnitFile()))
mu.Lock()
results = append(results, testResult{name: step.Name, err: err})
mu.Unlock()
Expand Down Expand Up @@ -143,7 +155,7 @@ func main() {
log.Println("All test groups passed")
}

func runTestBinary(testBinary, clusterName, namespace, labelFilter, skip, junitPath string, extraEnv []string) error {
func runTestBinary(testBinary, clusterName, namespace, labelFilter, skip, junitPath string) error {
ginkgoTimeout := os.Getenv("GINKGO_TIMEOUT")
if ginkgoTimeout == "" {
ginkgoTimeout = defaultGinkgoTimeout
Expand All @@ -167,7 +179,6 @@ func runTestBinary(testBinary, clusterName, namespace, labelFilter, skip, junitP
fmt.Sprintf("E2E_HOSTED_CLUSTER_NAME=%s", clusterName),
fmt.Sprintf("E2E_HOSTED_CLUSTER_NAMESPACE=%s", namespace),
)
cmd.Env = append(cmd.Env, extraEnv...)

return cmd.Run()
}
Expand Down
12 changes: 9 additions & 3 deletions test/e2e/v2/lifecycle/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,20 +119,26 @@ func (a *AWSPlatformConfig) PostVersionRollout(ctx context.Context, cl crclient.
return nil
}

func (a *AWSPlatformConfig) TestMatrix(releaseImage string) TestMatrix {
func (a *AWSPlatformConfig) DefaultTestPlan() TestPlan {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The default plan's matrix and ClusterSpecs() have to stay in sync by hand — every variant referenced here needs a matching entry in ClusterSpecs(), and the PlatformConfig docstring says DefaultTestPlan selects "all known variants." Nothing enforces that today: the tests validate against a synthetic registry and never construct the real AWSPlatformConfig/AzurePlatformConfig, so a drift only surfaces once create-guests starts provisioning in CI. A small table test ranging over the real platform configs that asserts plan.Validate(ClusterSpecs(...)) == nil (and, if the docstring is meant literally, that every spec variant is referenced by the matrix) would catch it in milliseconds. Alternatively, soften the docstring if the default plan is intentionally a curated subset.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made the interface docs for DefaultTestPlan more precise (I think), as to introducing a stronger coupling between the matrix and the clusterspecs, I'd like to deal with that in a followup since I think we could start lifting out the specs into a more declarative form that will help address it more cleanly that I can really do here without increasing the scope of changes too far for now

return TestPlan{
Name: "aws-full",
Platform: "aws",
TestMatrix: a.TestMatrix(),
}
}

func (a *AWSPlatformConfig) TestMatrix() TestMatrix {
return TestMatrix{
Parallel: []TestGroup{
{
Name: "public",
Variant: "public",
LabelFilter: "!lifecycle || hosted-cluster-aws",
JUnitFile: "junit_public.xml",
},
{
Name: "karpenter",
Variant: "karpenter",
LabelFilter: "karpenter",
JUnitFile: "junit_karpenter.xml",
},
},
}
Expand Down
20 changes: 9 additions & 11 deletions test/e2e/v2/lifecycle/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -321,39 +321,42 @@ func (a *AzurePlatformConfig) postCreateExternalOIDC(ctx context.Context, cl crc
return nil
}

func (a *AzurePlatformConfig) TestMatrix(releaseImage string) TestMatrix {
func (a *AzurePlatformConfig) DefaultTestPlan() TestPlan {
return TestPlan{
Name: "azure-full",
Platform: "azure",
TestMatrix: a.TestMatrix(),
}
}

func (a *AzurePlatformConfig) TestMatrix() TestMatrix {
return TestMatrix{
Parallel: []TestGroup{
{
Name: "public",
Variant: "public",
LabelFilter: "self-managed-azure-public || nodepool-lifecycle || secret-encryption || control-plane-workloads || hosted-cluster-security || nodepool-osimagestream",
Skip: "KAS allowed CIDRs",
JUnitFile: "junit_self_managed_azure_public.xml",
},
{
Name: "private",
Variant: "private",
LabelFilter: "self-managed-azure-private || hosted-cluster-compliance",
JUnitFile: "junit_self_managed_azure_private.xml",
},
{
Name: "oauth-lb",
Variant: "oauth-lb",
LabelFilter: "self-managed-azure-oauth-lb || hosted-cluster-health || hosted-cluster-metrics || hosted-cluster-image-registry",
JUnitFile: "junit_self_managed_azure_oauth_lb.xml",
},
{
Name: "autoscaling",
Variant: "autoscaling",
LabelFilter: "nodepool-autoscaling",
JUnitFile: "junit_self_managed_azure_nodepool_autoscaling.xml",
},
{
Name: "external-oidc",
Variant: "external-oidc",
LabelFilter: "external-oidc || global-pull-secret",
JUnitFile: "junit_self_managed_azure_external_oidc.xml",
},
},
Sequential: []SequentialGroup{
Expand All @@ -364,20 +367,16 @@ func (a *AzurePlatformConfig) TestMatrix(releaseImage string) TestMatrix {
Name: "upgrade",
Variant: "upgrade",
LabelFilter: "control-plane-upgrade",
JUnitFile: "junit_lifecycle_upgrade.xml",
ExtraEnv: []string{fmt.Sprintf("E2E_LATEST_RELEASE_IMAGE=%s", releaseImage)},
},
{
Name: "control-plane-tls",
Variant: "upgrade",
LabelFilter: "control-plane-pki-operator",
JUnitFile: "junit_control_plane_tls.xml",
},
{
Name: "etcd-chaos",
Variant: "upgrade",
LabelFilter: "etcd-chaos",
JUnitFile: "junit_lifecycle_etcd_chaos.xml",
},
},
},
Expand Down Expand Up @@ -412,7 +411,6 @@ func (a *AzurePlatformConfig) DestroyArgs() []string {
}
}


func envOrDefault(key, defaultVal string) string {
if val := os.Getenv(key); val != "" {
return val
Expand Down
97 changes: 83 additions & 14 deletions test/e2e/v2/lifecycle/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,42 +5,106 @@ package lifecycle
import (
"context"
"crypto/sha256"
"errors"
"fmt"
"strings"

crclient "sigs.k8s.io/controller-runtime/pkg/client"
)

// ClusterSpec describes a single cluster to create for lifecycle tests.
type ClusterSpec struct {
Variant string
ExtraArgs []string
ReleaseImage string // override (empty = use default)
Variant string `json:"variant"`
ExtraArgs []string `json:"extraArgs,omitempty"`
ReleaseImage string `json:"releaseImage,omitempty"` // override (empty = use default)
}

// TestGroup describes one logical group of e2e tests to execute.
type TestGroup struct {
Name string
Variant string
LabelFilter string
Skip string
JUnitFile string
ExtraEnv []string
Name string `json:"name"`
Variant string `json:"variant"`
LabelFilter string `json:"labelFilter"`
Skip string `json:"skip,omitempty"`
}

// JUnitFile returns the deterministic JUnit XML filename for this
// test group, derived from the group name. It panics if the name
// contains path separators or traversal sequences; callers must
// validate the matrix before use.
func (g TestGroup) JUnitFile() string {
if err := validateGroupName(g.Name); err != nil {
panic(err.Error())
}
return fmt.Sprintf("junit_%s.xml", g.Name)
}

// validateGroupName checks that name is safe for use as a path
// component in JUnit filenames (no separators or traversal sequences).
func validateGroupName(name string) error {
if strings.Contains(name, "/") || strings.Contains(name, "\\") || strings.Contains(name, "..") {
return fmt.Errorf("invalid path component in test group name: %q", name)
}
return nil
}

// SequentialGroup runs its Steps one after another within a single
// goroutine. If any step fails, subsequent steps are skipped.
type SequentialGroup struct {
Name string
Steps []TestGroup
Name string `json:"name"`
Steps []TestGroup `json:"steps"`
}

// TestMatrix defines the full set of test groups for a platform.
// Parallel groups all run concurrently. Each SequentialGroup also
// runs concurrently with everything else, but its internal Steps
// run one after another.
type TestMatrix struct {
Parallel []TestGroup
Sequential []SequentialGroup
Parallel []TestGroup `json:"parallel,omitempty"`
Sequential []SequentialGroup `json:"sequential,omitempty"`
}

// Validate checks that all group names within the matrix are unique
// and safe for use as JUnit filename components.
func (m TestMatrix) Validate() error {
seen := make(map[string]bool)
var errs []error
check := func(name string) {
if err := validateGroupName(name); err != nil {
errs = append(errs, err)
}
if seen[name] {
errs = append(errs, fmt.Errorf("duplicate test group name: %q", name))
}
seen[name] = true
}
for _, g := range m.Parallel {
check(g.Name)
}
for _, sg := range m.Sequential {
for _, step := range sg.Steps {
check(step.Name)
}
}
return errors.Join(errs...)
}

// Variants returns the unique cluster variants referenced by the
// matrix. No ordering is guaranteed.
func (m TestMatrix) Variants() []string {
seen := make(map[string]bool)
for _, g := range m.Parallel {
seen[g.Variant] = true
}
for _, sg := range m.Sequential {
for _, step := range sg.Steps {
seen[step.Variant] = true
}
}
variants := make([]string, 0, len(seen))
for v := range seen {
variants = append(variants, v)
}
return variants
}

// ResolveVariants validates that every variant referenced by the test
Expand Down Expand Up @@ -117,8 +181,13 @@ type PlatformConfig interface {
// block the initial version rollout if applied earlier.
PostVersionRollout(ctx context.Context, cl crclient.WithWatch, namespace string, clusterNames map[string]string) error

// DefaultTestPlan returns the full test plan for this platform,
// selecting all variants returned by ClusterSpecs and the complete
// test matrix using those variants.
DefaultTestPlan() TestPlan

// TestMatrix returns the test groups for this platform.
TestMatrix(releaseImage string) TestMatrix
TestMatrix() TestMatrix

// SetupTestEnv sets platform-specific environment variables
// before test execution (e.g., reading subnet IDs from
Expand Down
Loading
Loading