fix: Phase 3 — Long-term improvements (SSH TOFU, EC2Client split, context propagation) + E2E tests#625
Merged
ArangoGutierrez merged 1 commit intoFeb 9, 2026
Conversation
There was a problem hiding this comment.
Pull request overview
Phase 3 of the 360-degree evaluation: adds long-term architectural improvements (context propagation, EC2 client interface split) plus provisioner resiliency and a new Ginkgo/Gomega E2E suite for AWS lifecycle validation.
Changes:
- Introduces Ginkgo/Gomega E2E tests and AWS environment fixtures for containerd+kubeadm and docker+nvidia runtime flows.
- Propagates
context.Contextthrough AWS provider lifecycle (New/Create/Delete/DryRun) and introduces a narrowEC2Clientinterface for testability. - Improves provisioner templates with retries and more resilient package/driver installation logic.
Reviewed changes
Copilot reviewed 24 out of 639 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_test.go | Adds Ginkgo suite bootstrap + env initialization for E2E tests |
| tests/aws_test.go | Implements AWS E2E table tests (create → provision → validate → cleanup) |
| tests/common/common.go | Adds helper UID generator for unique E2E resource names |
| tests/data/test_aws.yml | Adds AWS E2E environment fixture for containerd + kubeadm |
| tests/data/test_aws_docker.yml | Adds AWS E2E environment fixture for docker runtime |
| pkg/provisioner/templates/common.go | Makes apt installs more resilient with broader retries/repair |
| pkg/provisioner/templates/nv-driver.go | Adds arch detection + driver install fallback chain |
| pkg/provisioner/templates/docker.go | Wraps apt update with retry |
| pkg/provisioner/templates/containerd.go | Adds retry, installs “latest” by default, minor hardening |
| pkg/provisioner/templates/container-toolkit.go | Fixes duplicate Execute() and adds retry wrappers |
| pkg/provisioner/provisioner.go | Renames HostUrl→HostURL and updates kubeadm endpoint host wiring |
| internal/aws/ec2_client.go | Adds composed EC2 interfaces to decompose a monolithic client |
| pkg/provider/aws/aws.go | Switches EC2 client to interface + adds ctx to New |
| pkg/provider/aws/create.go | Threads ctx through EC2 create calls and updates ImageID field usage |
| pkg/provider/aws/delete.go | Threads ctx through EC2 delete calls and extends termination waiter |
| pkg/provider/aws/dryrun.go | Uses ctx for dryrun calls + updates ImageID field usage |
| api/holodeck/v1alpha1/types.go | Renames HostUrl/ImageId/OwnerId fields to HostURL/ImageID/OwnerID |
| api/holodeck/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy for renamed API fields |
| cmd/cli/create/create.go | Adds ctx plumbing into AWS create flow + HostURL rename usage |
| cmd/cli/delete/delete.go | Adds ctx plumbing into AWS delete flow |
| cmd/cli/dryrun/dryrun.go | Adds ctx plumbing into AWS dryrun flow + HostURL rename usage |
| cmd/action/ci/entrypoint.go | Adds ctx plumbing into AWS create flow for GitHub Action |
| cmd/action/ci/cleanup.go | Adds ctx plumbing into AWS delete flow for GitHub Action |
| go.mod | Bumps Go version and adds Ginkgo/Gomega dependencies for E2E |
Comment on lines
+36
to
+41
| func TestMain(t *testing.T) { | ||
| suiteName := "E2E Holodeck" | ||
|
|
||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, suiteName) | ||
| } |
There was a problem hiding this comment.
TestMain has special meaning in Go (func TestMain(m *testing.M)), so naming a regular Ginkgo entry test TestMain(t *testing.T) is confusing and can block future use of an actual TestMain. Rename this to something like TestE2E(t *testing.T) (or switch to the real TestMain(m *testing.M) signature if you intended process-level setup/teardown).
Comment on lines
+115
to
+126
| defer func() { | ||
| if p.Client != nil { | ||
| session, err := p.Client.NewSession() | ||
| if err == nil { | ||
| session.Close() | ||
| if err := p.Client.Close(); err != nil { | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to close ssh client") | ||
| } | ||
| } | ||
| p.Client = nil | ||
| } | ||
| }() |
There was a problem hiding this comment.
The SSH client is only closed when NewSession() succeeds. If NewSession() fails, this leaks the underlying connection and can leave resources open across retries/tests. Close p.Client unconditionally when it’s non-nil; only gate session.Close() on session creation success (and note that you don’t need a session just to close the client).
Comment on lines
+22
to
+32
| "log" | ||
| ) | ||
|
|
||
| func GenerateUID() string { | ||
| b := make([]byte, 4) | ||
| _, err := rand.Read(b) | ||
| if err != nil { | ||
| log.Fatalf("failed to generate secure random UID: %v", err) | ||
| } | ||
| return hex.EncodeToString(b) | ||
| } |
There was a problem hiding this comment.
This helper hard-exits the entire test process via log.Fatalf, which bypasses Ginkgo failure reporting and can skip deferred cleanup (potentially leaving AWS resources behind). Prefer returning (string, error) (or failing via Ginkgo in the caller) and consider increasing the entropy beyond 4 bytes to reduce collision risk for resource naming under parallel/CI runs.
Suggested change
| "log" | |
| ) | |
| func GenerateUID() string { | |
| b := make([]byte, 4) | |
| _, err := rand.Read(b) | |
| if err != nil { | |
| log.Fatalf("failed to generate secure random UID: %v", err) | |
| } | |
| return hex.EncodeToString(b) | |
| } | |
| "fmt" | |
| ) | |
| func GenerateUID() (string, error) { | |
| b := make([]byte, 16) | |
| if _, err := rand.Read(b); err != nil { | |
| return "", fmt.Errorf("failed to generate secure random UID: %w", err) | |
| } | |
| return hex.EncodeToString(b), nil | |
| } |
| type: g4dn.xlarge | ||
| region: us-west-1 | ||
| ingressIpRanges: | ||
| - "0.0.0.0/0" |
There was a problem hiding this comment.
Allowing SSH/K8s ingress from 0.0.0.0/0 is a high-risk default even for E2E, since it opens the instance to the public internet. Prefer restricting this to the CI runner’s egress IP (or a tightly scoped CIDR provided via CI/env templating) so tests don’t create broadly exposed infrastructure.
Suggested change
| - "0.0.0.0/0" | |
| - "192.0.2.1/32" # restricted example CIDR; override with CI runner egress IP in real use |
| type: g4dn.xlarge | ||
| region: us-west-1 | ||
| ingressIpRanges: | ||
| - "0.0.0.0/0" |
There was a problem hiding this comment.
Allowing SSH ingress from 0.0.0.0/0 is a high-risk default even for E2E. Prefer restricting this to the CI runner’s egress IP (or a tightly scoped CIDR provided via CI/env templating) so tests don’t create broadly exposed infrastructure.
Suggested change
| - "0.0.0.0/0" | |
| - "${HOLODECK_E2E_INGRESS_CIDR}" |
| install_packages_with_retry cuda-drivers | ||
| # Install the latest cuda-keyring | ||
| wget -q "https://developer.download.nvidia.com/compute/cuda/repos/${distribution}/${NVIDIA_ARCH}/cuda-keyring_1.1-1_all.deb" \ | ||
| || wget -q "https://developer.download.nvidia.com/compute/cuda/repos/${distribution}/${NVIDIA_ARCH}/cuda-keyring_1.0-1_all.deb" |
There was a problem hiding this comment.
If both wget attempts fail, the dpkg -i cuda-keyring_*.deb glob may not match anything (or may match an unexpected file), causing a misleading failure path. Add an explicit check that a keyring .deb was downloaded before running dpkg, and emit a clear error if neither URL is available for the detected distro/arch.
Suggested change
| || wget -q "https://developer.download.nvidia.com/compute/cuda/repos/${distribution}/${NVIDIA_ARCH}/cuda-keyring_1.0-1_all.deb" | |
| || wget -q "https://developer.download.nvidia.com/compute/cuda/repos/${distribution}/${NVIDIA_ARCH}/cuda-keyring_1.0-1_all.deb" | |
| # Ensure that a cuda-keyring .deb was actually downloaded before installing | |
| if ! ls cuda-keyring_*.deb >/dev/null 2>&1; then | |
| echo "ERROR: failed to download cuda-keyring for ${distribution}/${NVIDIA_ARCH}" >&2 | |
| exit 1 | |
| fi |
Comment on lines
55
to
59
| func New(log *logger.FunLogger, keyPath, userName, hostURL string) (*Provisioner, error) { | ||
| client, err := connectOrDie(keyPath, userName, hostURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to %s: %v", hostUrl, err) | ||
| return nil, fmt.Errorf("failed to connect to %s: %v", hostURL, err) | ||
| } |
There was a problem hiding this comment.
This wraps the underlying error using %v, which prevents callers from unwrapping/inspecting the root cause. Prefer wrapping with %w (and keep the contextual host string) so higher-level code can use errors.Is/As effectively.
ArangoGutierrez
force-pushed
the
fix/phase3-longterm-improvements
branch
from
February 9, 2026 13:15
b500ab1 to
0938b0c
Compare
Pull Request Test Coverage Report for Build 21831869854Details
💛 - Coveralls |
ArangoGutierrez
enabled auto-merge (squash)
February 9, 2026 13:29
ArangoGutierrez
force-pushed
the
fix/phase3-longterm-improvements
branch
4 times, most recently
from
February 9, 2026 15:42
cc1ec92 to
f9b3f41
Compare
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
force-pushed
the
fix/phase3-longterm-improvements
branch
from
February 9, 2026 15:47
f9b3f41 to
13229b1
Compare
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 12, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
- Replace ssh.InsecureIgnoreHostKey() with Trust-On-First-Use (TOFU) pattern that records host keys on first connection and verifies on subsequent ones, eliminating MITM vulnerability - Host keys cached in ~/.cache/holodeck/known_hosts with 0600 permissions Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3 of the 360-degree codebase evaluation. Addresses long-term architectural improvements.
Architectural Improvements
context.Contextthrough all AWS SDK calls for proper cancellation and timeout supportProvisioner Resilience
E2E Test Suite (NEW)
Test Plan
go vet ./...passes-s -S(DCO + SSH)Made with Cursor