From aa03cabf372370d9fc58ba95078388b13cf46797 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Mon, 11 May 2026 12:54:04 +1000 Subject: [PATCH] =?UTF-8?q?fix(ddil):=20CI=20green-up=20=E2=80=94=20instan?= =?UTF-8?q?ce-type=20discovery,=20gotestsum,=20EC2=20envelope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes to take the DDIL E2E suite from "scenarios flipped" to actually green on the tofu cluster. mulga-siv-56. 1. Witness auto-discovers the smallest registered instance type (DescribeInstanceTypes, sort by memory then vCPUs). The hard-coded t2.micro default returned InvalidInstanceType on every Scenario A/B/C launch because the cluster doesn't register that type. DDIL_WITNESS_- INSTANCE_TYPE stays as an operator override. 2. e2e-ddil.yml uses gotestsum so the Actions UI shows per-test PASS/FAIL/ SKIP lines instead of a JSON wall. results.json artifact preserved for future flake-rate dashboarding. 3. GenerateEC2ErrorResponse emits the real EC2 query-API envelope ... instead of the IAM-shaped . aws-sdk-go v1's ec2query handler rejects the latter with SerializationError and drops the code, leaving Go callers without an awserr.Code() to inspect. New SDK round-trip test in gateway_test.go guards the regression. --- .github/workflows/e2e-ddil.yml | 15 +++++-- spinifex/gateway/gateway.go | 27 ++++++++----- spinifex/gateway/gateway_test.go | 65 ++++++++++++++++++++++++++++--- tests/e2e/ddil/harness/witness.go | 61 +++++++++++++++++++++++++++-- 4 files changed, 146 insertions(+), 22 deletions(-) diff --git a/.github/workflows/e2e-ddil.yml b/.github/workflows/e2e-ddil.yml index cfc6edddc..93b0089fd 100644 --- a/.github/workflows/e2e-ddil.yml +++ b/.github/workflows/e2e-ddil.yml @@ -53,6 +53,13 @@ jobs: sparse-checkout: scripts/tofu-cluster path: mulga + - name: Install gotestsum + run: | + # Idempotent: re-running `go install` on a cached binary is a no-op. + # Pinned tag keeps Actions output format stable across runs. + go install gotest.tools/gotestsum@v1.12.0 + echo "${HOME}/go/bin" >> "${GITHUB_PATH}" + - name: Build Install Artifacts working-directory: mulga/scripts/tofu-cluster run: | @@ -115,6 +122,8 @@ jobs: mkdir -p "${RUNNER_TEMP}/ddil-results" cd "${GITHUB_WORKSPACE}/spinifex" + # gotestsum: human-readable PASS/FAIL/SKIP lines in Actions stdout + # while still emitting the machine-readable jsonfile artifact. DDIL_NODES="${NODE_IPS}" \ DDIL_SSH_USER="${SSH_USER}" \ DDIL_SSH_KEY="${SSH_KEY}" \ @@ -122,9 +131,9 @@ jobs: AWS_REGION="${REGION}" \ AWS_ACCESS_KEY_ID="${AWS_ACCESS_KEY_ID}" \ AWS_SECRET_ACCESS_KEY="${AWS_SECRET_ACCESS_KEY}" \ - go test -tags=e2e -timeout 45m -json \ - ./tests/e2e/ddil/scenarios/... \ - | tee "${RUNNER_TEMP}/ddil-results/results.json" + gotestsum --jsonfile "${RUNNER_TEMP}/ddil-results/results.json" \ + --format testname \ + -- -tags=e2e -timeout 45m ./tests/e2e/ddil/scenarios/... - name: Upload DDIL Results if: always() diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 9f86a20f9..57fa2b06b 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -61,15 +61,22 @@ var supportedServices = map[string]bool{ "spinifex": true, } -const xmlnsEC2 = "http://ec2.amazonaws.com/doc/2016-11-15/" - -type ErrorResponse struct { - XMLName xml.Name `xml:"http://ec2.amazonaws.com/doc/2016-11-15/ ErrorResponse"` - Errors Errors `xml:"Errors"` - RequestID string `xml:"RequestID"` +// EC2ErrorResponse is the EC2 query-API error envelope. +// +// Real AWS EC2 emits `......` +// for error replies. aws-sdk-go v1's ec2query response handler is strict — it +// rejects the IAM-style `` envelope with SerializationError and +// drops the underlying error code, leaving callers without an awserr.Code() to +// inspect. The cluster-unavailable bailout in writeClusterUnavailable already +// uses this shape inline; this type is the structural equivalent for the +// generic error path. +type EC2ErrorResponse struct { + XMLName xml.Name `xml:"Response"` + Errors EC2Errors `xml:"Errors"` + RequestID string `xml:"RequestID"` } -type Errors struct { +type EC2Errors struct { Error ErrorDetail `xml:"Error"` } @@ -423,8 +430,8 @@ func ParseAWSQueryArgs(query string) (map[string]string, error) { } func GenerateEC2ErrorResponse(code, message, requestID string) (output []byte) { - errorXml := ErrorResponse{ - Errors: Errors{ + errorXml := EC2ErrorResponse{ + Errors: EC2Errors{ Error: ErrorDetail{ Code: code, Message: errors.New(message), @@ -437,7 +444,7 @@ func GenerateEC2ErrorResponse(code, message, requestID string) (output []byte) { if err != nil { slog.Error("Failed to build XML", "error", err) - return []byte(xml.Header + `InternalErrorInternal error` + requestID + ``) + return []byte(xml.Header + `InternalErrorInternal error` + requestID + ``) } // Add XML header diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index e3cf06fcf..2223fd6a6 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -12,6 +12,11 @@ import ( "strings" "testing" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" + awscreds "github.com/aws/aws-sdk-go/aws/credentials" + awssession "github.com/aws/aws-sdk-go/aws/session" + awsec2 "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/predastore/ratelimit" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" @@ -72,9 +77,11 @@ func TestGenerateEC2ErrorResponse_Structure(t *testing.T) { // Verify request ID assert.Contains(t, xmlStr, ""+tc.requestID+"") - // Verify root element - assert.Contains(t, xmlStr, ``) - assert.Contains(t, xmlStr, "") + // Verify root element — EC2 query API uses , not + // ; aws-sdk-go v1 rejects the latter with + // SerializationError. + assert.Contains(t, xmlStr, "") + assert.Contains(t, xmlStr, "") // Verify Errors wrapper assert.Contains(t, xmlStr, "") @@ -203,7 +210,7 @@ func TestErrorHandler_UnknownError(t *testing.T) { xmlStr := string(body) // Unknown errors should be remapped to InternalError assert.Contains(t, xmlStr, "InternalError") - assert.Contains(t, xmlStr, ``) + assert.Contains(t, xmlStr, "") assert.Contains(t, xmlStr, "") } @@ -222,7 +229,7 @@ func TestErrorHandler_EC2Service(t *testing.T) { body, _ := io.ReadAll(resp.Body) xmlStr := string(body) - assert.Contains(t, xmlStr, ``) + assert.Contains(t, xmlStr, "") assert.Contains(t, xmlStr, "") assert.Contains(t, xmlStr, "InvalidParameterValue") } @@ -1156,7 +1163,7 @@ func TestWriteThrottleError_EC2(t *testing.T) { assert.Equal(t, 503, resp.StatusCode) assert.Equal(t, "application/xml", resp.Header.Get("Content-Type")) assert.Contains(t, string(body), "RequestLimitExceeded") - assert.Contains(t, string(body), ``) + assert.Contains(t, string(body), "") } func TestWriteThrottleError_IAM(t *testing.T) { @@ -1338,3 +1345,49 @@ func TestRequest_ClusterUnavailableClosedConn(t *testing.T) { assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) } + +// TestGenerateEC2ErrorResponse_SDKRoundTrip serves the EC2 error envelope from +// an httptest server, points an aws-sdk-go v1 EC2 client at it, and asserts +// the SDK surfaces the code via awserr.Error.Code() — not SerializationError. +// +// This is the regression guard for the bug behind mulga-siv-56 §3: aws-sdk-go +// v1's ec2query response handler rejects the IAM `` envelope +// and discards the embedded code, leaving Go callers without a parseable +// AWS error. +func TestGenerateEC2ErrorResponse_SDKRoundTrip(t *testing.T) { + const wantCode = "InvalidInstanceType" + const wantMessage = "The instance type 't2.micro' is not supported in this region." + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + body := GenerateEC2ErrorResponse(wantCode, wantMessage, "req-sdk-roundtrip") + w.Header().Set("Content-Type", "application/xml") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write(body) + })) + defer srv.Close() + + sess, err := awssession.NewSession(&aws.Config{ + Region: aws.String("us-east-1"), + Endpoint: aws.String(srv.URL), + Credentials: awscreds.NewStaticCredentials("AKIA-TEST", "secret", ""), + DisableSSL: aws.Bool(true), + // Suppress the default retry loop — error responses are not retryable + // here and waiting them out wastes test time. + MaxRetries: aws.Int(0), + }) + require.NoError(t, err) + + client := awsec2.New(sess) + _, err = client.RunInstances(&awsec2.RunInstancesInput{ + ImageId: aws.String("ami-test"), + InstanceType: aws.String("t2.micro"), + MinCount: aws.Int64(1), + MaxCount: aws.Int64(1), + }) + require.Error(t, err) + + var awsErr awserr.Error + require.True(t, errors.As(err, &awsErr), "expected awserr.Error, got %T: %v", err, err) + assert.Equal(t, wantCode, awsErr.Code()) + assert.NotEqual(t, "SerializationError", awsErr.Code(), "SDK could not parse the envelope") +} diff --git a/tests/e2e/ddil/harness/witness.go b/tests/e2e/ddil/harness/witness.go index 2118e0bfd..c6e3b0419 100644 --- a/tests/e2e/ddil/harness/witness.go +++ b/tests/e2e/ddil/harness/witness.go @@ -13,6 +13,7 @@ import ( "net/http" "os" "regexp" + "sort" "strconv" "strings" "testing" @@ -55,7 +56,11 @@ type Witness struct { // - AWS_REGION (required) — passed to the AWS SDK client. // - DDIL_WITNESS_AMI (optional) — specific AMI ID; if unset, an // ami-ubuntu-* image is discovered at launch time. -// - DDIL_WITNESS_INSTANCE_TYPE (default t2.micro) — instance type. +// - DDIL_WITNESS_INSTANCE_TYPE (optional) — explicit instance type; if +// unset, the smallest registered type (by memory, then vCPUs) is +// auto-discovered at launch time. Auto-discovery beats hard-coding +// `t2.micro` because tofu clusters seed `*.nano` variants under +// names that change between releases. // - DDIL_WITNESS_KEY_NAME (default spinifex-key) — EC2 key pair name. // - DDIL_GUEST_SSH_USER (default ubuntu) — user for guest SSH. // - DDIL_GUEST_SSH_KEY (default cluster.SSHKeyPath) — private key for @@ -107,7 +112,7 @@ func NewWitness(cluster *Cluster, transport SSH) (*Witness, error) { guestSigner: guestSigner, guestUser: envDefault("DDIL_GUEST_SSH_USER", "ubuntu"), ami: os.Getenv("DDIL_WITNESS_AMI"), - instanceType: envDefault("DDIL_WITNESS_INSTANCE_TYPE", "t2.micro"), + instanceType: os.Getenv("DDIL_WITNESS_INSTANCE_TYPE"), keyName: envDefault("DDIL_WITNESS_KEY_NAME", "spinifex-key"), }, nil } @@ -141,6 +146,10 @@ func LaunchWitnessVM(ctx context.Context, w *Witness, host Node) (*WitnessVM, er if err != nil { return nil, err } + instanceType, err := w.resolveInstanceType(ctx) + if err != nil { + return nil, err + } userData := base64.StdEncoding.EncodeToString(cloudInitWitness) @@ -148,7 +157,7 @@ func LaunchWitnessVM(ctx context.Context, w *Witness, host Node) (*WitnessVM, er for attempt := 1; attempt <= maxPlacementAttempts; attempt++ { out, err := w.ec2.RunInstancesWithContext(ctx, &ec2.RunInstancesInput{ ImageId: aws.String(ami), - InstanceType: aws.String(w.instanceType), + InstanceType: aws.String(instanceType), MinCount: aws.Int64(1), MaxCount: aws.Int64(1), KeyName: aws.String(w.keyName), @@ -251,6 +260,52 @@ func (w *Witness) resolveAMI(ctx context.Context) (string, error) { return w.ami, nil } +// resolveInstanceType returns the witness instance type, caching the result. +// When DDIL_WITNESS_INSTANCE_TYPE is set it is honoured verbatim; otherwise +// DescribeInstanceTypes is consulted and the smallest registered type — by +// memory first, vCPUs as tiebreaker — is selected. The sort key is +// quantitative on purpose: naming conventions (`t2.nano`, `c6i.large`, …) +// shift between cluster releases, but smallest-by-resource always picks the +// cheapest valid launch target. +func (w *Witness) resolveInstanceType(ctx context.Context) (string, error) { + if w.instanceType != "" { + return w.instanceType, nil + } + out, err := w.ec2.DescribeInstanceTypesWithContext(ctx, &ec2.DescribeInstanceTypesInput{}) + if err != nil { + return "", fmt.Errorf("ddil harness: DescribeInstanceTypes: %w", err) + } + if len(out.InstanceTypes) == 0 { + return "", errors.New("ddil harness: cluster registered no instance types") + } + sort.Slice(out.InstanceTypes, func(i, j int) bool { + a, b := out.InstanceTypes[i], out.InstanceTypes[j] + var aMem, bMem int64 + if a.MemoryInfo != nil { + aMem = aws.Int64Value(a.MemoryInfo.SizeInMiB) + } + if b.MemoryInfo != nil { + bMem = aws.Int64Value(b.MemoryInfo.SizeInMiB) + } + if aMem != bMem { + return aMem < bMem + } + var aCPU, bCPU int64 + if a.VCpuInfo != nil { + aCPU = aws.Int64Value(a.VCpuInfo.DefaultVCpus) + } + if b.VCpuInfo != nil { + bCPU = aws.Int64Value(b.VCpuInfo.DefaultVCpus) + } + return aCPU < bCPU + }) + w.instanceType = aws.StringValue(out.InstanceTypes[0].InstanceType) + if w.instanceType == "" { + return "", errors.New("ddil harness: smallest instance type has empty name") + } + return w.instanceType, nil +} + func (w *Witness) waitForRunning(ctx context.Context, id string, timeout time.Duration) error { deadline := time.Now().Add(timeout) const interval = 2 * time.Second