Skip to content

Commit b19752d

Browse files
joecklaujoecklau
authored
Merge pull request #43 from solutionforest/develop
Promote Docker Sandboxes host-trust fixes to main
2 parents ebbc10c + 1b8f4db commit b19752d

7 files changed

Lines changed: 692 additions & 60 deletions

File tree

‎internal/pool/host_trust_test.go‎

Lines changed: 243 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"github.com/solutionforest/ephemeral-action-runner/internal/config"
2323
gh "github.com/solutionforest/ephemeral-action-runner/internal/github"
2424
"github.com/solutionforest/ephemeral-action-runner/internal/hosttrust"
25+
"github.com/solutionforest/ephemeral-action-runner/internal/logging"
26+
poolstate "github.com/solutionforest/ephemeral-action-runner/internal/pool/state"
2527
"github.com/solutionforest/ephemeral-action-runner/internal/provider"
2628
)
2729

@@ -396,8 +398,8 @@ func TestHostTrustReconciliationFencesWhenTransportVerificationFails(t *testing.
396398
if activator.calls != 0 {
397399
t.Fatalf("activation calls = %d, want zero while fencing the unhealthy registered runner", activator.calls)
398400
}
399-
if activator.verifyCalls != 1 {
400-
t.Fatalf("runtime verification calls = %d, want one before fencing the unhealthy registered runner", activator.verifyCalls)
401+
if activator.verifyCalls != 0 {
402+
t.Fatalf("common runtime verification calls = %d, want zero when a dedicated host-trust verifier exists", activator.verifyCalls)
401403
}
402404
if activator.verifyHostTrustCalls != 1 {
403405
t.Fatalf("provider host-trust verification calls = %d, want one before fencing the unhealthy registered runner", activator.verifyHostTrustCalls)
@@ -438,8 +440,8 @@ func TestHostTrustReconciliationDoesNotReactivateHealthyTransport(t *testing.T)
438440
if activator.calls != 0 {
439441
t.Fatalf("host trust transport activations = %d, want zero for healthy current-generation transport", activator.calls)
440442
}
441-
if activator.verifyCalls != 1 {
442-
t.Fatalf("runtime verification calls = %d, want one for healthy current-generation transport", activator.verifyCalls)
443+
if activator.verifyCalls != 0 {
444+
t.Fatalf("common runtime verification calls = %d, want zero when a dedicated host-trust verifier exists", activator.verifyCalls)
443445
}
444446
if activator.verifyHostTrustCalls != 1 {
445447
t.Fatalf("provider host-trust verification calls = %d, want one for healthy current-generation transport", activator.verifyHostTrustCalls)
@@ -478,8 +480,8 @@ func TestHostTrustReconciliationFencesBusyRunnerWhenTransportVerificationFails(t
478480
if activator.calls != 0 {
479481
t.Fatalf("host trust transport activations = %d, want zero for failed verification", activator.calls)
480482
}
481-
if activator.verifyCalls != 1 {
482-
t.Fatalf("runtime verification calls = %d, want one before fencing the busy runner", activator.verifyCalls)
483+
if activator.verifyCalls != 0 {
484+
t.Fatalf("common runtime verification calls = %d, want zero when a dedicated host-trust verifier exists", activator.verifyCalls)
483485
}
484486
if activator.verifyHostTrustCalls != 1 {
485487
t.Fatalf("provider host-trust verification calls = %d, want one before fencing the busy runner", activator.verifyHostTrustCalls)
@@ -492,6 +494,241 @@ func TestHostTrustReconciliationFencesBusyRunnerWhenTransportVerificationFails(t
492494
}
493495
}
494496

497+
func TestHostTrustReconciliationBusyRunnerIgnoresJobMutableGeneralRuntimeState(t *testing.T) {
498+
fake := &fakeProvider{instances: []provider.Instance{{Name: "runner-1", ProviderID: "fake:runner-1", State: "running"}}}
499+
activator := &hostTrustVerifyingLifecycle{
500+
activatingLifecycle: &activatingLifecycle{
501+
Lifecycle: provider.AdaptLegacy(fake, false),
502+
verifyErr: errors.New("workflow-created Docker client configuration is present"),
503+
},
504+
}
505+
github := &fakeGitHub{runner: gh.Runner{Name: "runner-1", ID: 42, Status: "online", Busy: true}, found: true}
506+
manager := Manager{
507+
Config: config.Config{
508+
Provider: config.ProviderConfig{Type: "docker-sandboxes"},
509+
Image: config.ImageConfig{HostTrustMode: config.HostTrustModeOverlay, HostTrustScopes: []string{"system"}},
510+
},
511+
Provider: fake,
512+
Lifecycle: activator,
513+
GitHub: github,
514+
}
515+
var console bytes.Buffer
516+
logDirectory := t.TempDir()
517+
manager.Config.Logging.Directory = logDirectory
518+
runtime, err := logging.NewRuntime(logging.Options{Directory: logDirectory, ManagerSinks: logging.SinkConsole, Stdout: &console, Stderr: &console})
519+
if err != nil {
520+
t.Fatal(err)
521+
}
522+
defer runtime.Close()
523+
manager.Logging = runtime
524+
current := hosttrust.Snapshot{Generation: "g1", HostOS: "windows", Scopes: []string{"system"}, Certificates: []hosttrust.Certificate{{Name: "root.crt", PEM: []byte("pem")}}, CollectedAt: time.Now().UTC()}
525+
active := map[string]ProvisionedInstance{"runner-1": {Name: "runner-1", ProviderID: "fake:runner-1", RunnerID: 42, HostTrustGeneration: "g1", ProviderOwned: true, Phase: LifecycleReady}}
526+
busyHandoff := make(map[string]bool)
527+
528+
manager.reconcileHostTrustRunners(context.Background(), active, current, busyHandoff)
529+
manager.reconcileHostTrustRunners(context.Background(), active, current, busyHandoff)
530+
531+
if got := active["runner-1"].Phase; got != LifecycleReady {
532+
t.Fatalf("runner phase = %s, want %s", got, LifecycleReady)
533+
}
534+
if activator.verifyCalls != 0 {
535+
t.Fatalf("common runtime verification calls = %d, want zero for job-mutable runtime state", activator.verifyCalls)
536+
}
537+
if activator.verifyHostTrustCalls != 2 {
538+
t.Fatalf("provider host-trust verification calls = %d, want one read-only transport check per reconciliation", activator.verifyHostTrustCalls)
539+
}
540+
if got := len(hostTrustLeaseInputs(fake)); got != 1 {
541+
t.Fatalf("busy handoff lease writes = %d, want one bounded lease", got)
542+
}
543+
if got := atomic.LoadInt32(&github.deleteCalls); got != 0 {
544+
t.Fatalf("GitHub registration fence calls = %d, want zero for healthy busy transport", got)
545+
}
546+
if output := console.String(); strings.Contains(output, "host trust transport verification warning") || strings.Contains(output, "host trust registration fencing warning") {
547+
t.Fatalf("healthy busy reconciliation emitted transport or fencing warning: %q", output)
548+
}
549+
}
550+
551+
func TestHostTrustReconciliationActivatorOnlyFallsBackToCommonRuntimeVerification(t *testing.T) {
552+
fake := &fakeProvider{instances: []provider.Instance{{Name: "runner-1", ProviderID: "fake:runner-1", State: "running"}}}
553+
activator := &activatingLifecycle{
554+
Lifecycle: provider.AdaptLegacy(fake, false),
555+
verifyErr: errors.New("runtime unavailable"),
556+
}
557+
github := &fakeGitHub{runner: gh.Runner{Name: "runner-1", ID: 42, Status: "online"}, found: true}
558+
manager := Manager{
559+
Config: config.Config{
560+
Image: config.ImageConfig{HostTrustMode: config.HostTrustModeOverlay, HostTrustScopes: []string{"system"}},
561+
},
562+
Provider: fake,
563+
Lifecycle: activator,
564+
GitHub: github,
565+
}
566+
current := hosttrust.Snapshot{Generation: "g1", HostOS: "linux", Scopes: []string{"system"}, Certificates: []hosttrust.Certificate{{Name: "root.crt", PEM: []byte("pem")}}, CollectedAt: time.Now().UTC()}
567+
active := map[string]ProvisionedInstance{"runner-1": {Name: "runner-1", ProviderID: "fake:runner-1", RunnerID: 42, HostTrustGeneration: "g1", ProviderOwned: true, Phase: LifecycleReady}}
568+
569+
manager.reconcileHostTrustRunners(context.Background(), active, current, make(map[string]bool))
570+
571+
if activator.verifyCalls != 1 {
572+
t.Fatalf("common runtime verification calls = %d, want one fallback verification", activator.verifyCalls)
573+
}
574+
if got := active["runner-1"].Phase; got != LifecycleQuarantined {
575+
t.Fatalf("runner phase = %s, want %s", got, LifecycleQuarantined)
576+
}
577+
if got := atomic.LoadInt32(&github.deleteCalls); got != 1 {
578+
t.Fatalf("GitHub registration fence calls = %d, want one after fallback verification failure", got)
579+
}
580+
}
581+
582+
func TestDurableHostTrustQuarantineDoesNotRepeatBusyFenceAndRetiresWhenIdle(t *testing.T) {
583+
manager, store, name := readyLifecycleManager(t)
584+
fake := &fakeProvider{instances: []provider.Instance{{Name: name, ProviderID: "docker:ready-id", State: "running"}}}
585+
activator := &hostTrustVerifyingLifecycle{
586+
activatingLifecycle: &activatingLifecycle{Lifecycle: provider.AdaptLegacy(fake, false)},
587+
verifyHostTrustErr: errors.New("relay marker unavailable"),
588+
}
589+
busyRunner := gh.Runner{Name: name, ID: 42, Status: "online", Busy: true}
590+
github := &fakeGitHub{
591+
runner: busyRunner,
592+
found: true,
593+
listRunners: []gh.Runner{busyRunner},
594+
deleteErr: errors.New("runner is currently running a job"),
595+
}
596+
manager.Config.Image = config.ImageConfig{HostTrustMode: config.HostTrustModeOverlay, HostTrustScopes: []string{"system"}}
597+
manager.Provider = fake
598+
manager.Lifecycle = activator
599+
manager.GitHub = github
600+
current := hosttrust.Snapshot{Generation: "g1", HostOS: "windows", Scopes: []string{"system"}, Certificates: []hosttrust.Certificate{{Name: "root.crt", PEM: []byte("pem")}}, CollectedAt: time.Now().UTC()}
601+
active := map[string]ProvisionedInstance{name: {Name: name, ProviderID: "docker:ready-id", RunnerID: 42, HostTrustGeneration: "g1", ProviderOwned: true, Phase: LifecycleReady}}
602+
603+
manager.reconcileHostTrustRunners(context.Background(), active, current, make(map[string]bool))
604+
if got := atomic.LoadInt32(&github.deleteCalls); got != 1 {
605+
t.Fatalf("initial GitHub registration fence calls = %d, want 1", got)
606+
}
607+
if got := active[name].Phase; got != LifecycleQuarantined {
608+
t.Fatalf("phase after failed busy fence = %s, want %s", got, LifecycleQuarantined)
609+
}
610+
611+
var err error
612+
active, err = manager.reconcilePhysicalPool(context.Background(), active, true)
613+
if err != nil {
614+
t.Fatal(err)
615+
}
616+
manager.reconcileHostTrustRunners(context.Background(), active, current, make(map[string]bool))
617+
if got := active[name].Phase; got != LifecycleQuarantined {
618+
t.Fatalf("phase after busy reconciliation = %s, want durable quarantine", got)
619+
}
620+
if got := atomic.LoadInt32(&github.deleteCalls); got != 1 {
621+
t.Fatalf("GitHub registration fence calls after busy reconciliation = %d, want no repeat", got)
622+
}
623+
if activator.verifyHostTrustCalls != 1 {
624+
t.Fatalf("provider host-trust verification calls = %d, want no repeat after durable quarantine", activator.verifyHostTrustCalls)
625+
}
626+
record, err := store.Read(context.Background(), name)
627+
if err != nil {
628+
t.Fatal(err)
629+
}
630+
if record.Phase != poolstate.PhaseQuarantined {
631+
t.Fatalf("durable phase = %s, want %s", record.Phase, poolstate.PhaseQuarantined)
632+
}
633+
634+
idleRunner := busyRunner
635+
idleRunner.Busy = false
636+
github.runner = idleRunner
637+
github.listRunners = []gh.Runner{idleRunner}
638+
github.deleteErr = nil
639+
github.deleteFunc = func(context.Context, int64) error {
640+
github.found = false
641+
github.listRunners = nil
642+
return nil
643+
}
644+
active, err = manager.reconcilePhysicalPool(context.Background(), active, true)
645+
if err != nil {
646+
t.Fatal(err)
647+
}
648+
if _, found := active[name]; found {
649+
t.Fatalf("durably quarantined idle runner remains active: %#v", active[name])
650+
}
651+
if got := atomic.LoadInt32(&github.deleteCalls); got != 2 {
652+
t.Fatalf("total GitHub deletion calls = %d, want initial busy fence plus idle cleanup", got)
653+
}
654+
if got := atomic.LoadInt32(&fake.deleteCalls); got != 1 {
655+
t.Fatalf("provider deletion calls = %d, want one exact idle cleanup", got)
656+
}
657+
record, err = store.Read(context.Background(), name)
658+
if err != nil {
659+
t.Fatal(err)
660+
}
661+
if record.Phase != poolstate.PhaseTombstoned {
662+
t.Fatalf("durable phase after idle cleanup = %s, want %s", record.Phase, poolstate.PhaseTombstoned)
663+
}
664+
}
665+
666+
func TestReconciliationHydratesDurableBusyQuarantineWithoutReAdoption(t *testing.T) {
667+
manager, store, name := readyLifecycleManager(t)
668+
if _, err := store.Transition(context.Background(), name, poolstate.Transition{Action: poolstate.ActionQuarantine, Reason: "host trust transport unavailable"}); err != nil {
669+
t.Fatal(err)
670+
}
671+
fake := &fakeProvider{instances: []provider.Instance{{Name: name, ProviderID: "docker:ready-id", State: "running"}}}
672+
busyRunner := gh.Runner{Name: name, ID: 42, Status: "online", Busy: true}
673+
github := &fakeGitHub{runner: busyRunner, found: true, listRunners: []gh.Runner{busyRunner}}
674+
manager.Provider = fake
675+
manager.Lifecycle = provider.AdaptLegacy(fake, false)
676+
manager.GitHub = github
677+
678+
active, err := manager.reconcilePhysicalPool(context.Background(), nil, true)
679+
if err != nil {
680+
t.Fatal(err)
681+
}
682+
if got := active[name].Phase; got != LifecycleQuarantined {
683+
t.Fatalf("hydrated phase = %s, want %s", got, LifecycleQuarantined)
684+
}
685+
if got := atomic.LoadInt32(&github.deleteCalls); got != 0 {
686+
t.Fatalf("GitHub deletion calls = %d, want zero while hydrated quarantine is busy", got)
687+
}
688+
if got := atomic.LoadInt32(&fake.deleteCalls); got != 0 {
689+
t.Fatalf("provider deletion calls = %d, want zero while hydrated quarantine is busy", got)
690+
}
691+
record, err := store.Read(context.Background(), name)
692+
if err != nil {
693+
t.Fatal(err)
694+
}
695+
if record.Phase != poolstate.PhaseQuarantined {
696+
t.Fatalf("durable phase = %s, want %s", record.Phase, poolstate.PhaseQuarantined)
697+
}
698+
}
699+
700+
func TestReconciliationRecoversTransientQuarantineWhenDurableStateIsReady(t *testing.T) {
701+
manager, store, name := readyLifecycleManager(t)
702+
fake := &fakeProvider{instances: []provider.Instance{{Name: name, ProviderID: "docker:ready-id", State: "running"}}}
703+
idleRunner := gh.Runner{Name: name, ID: 42, Status: "online"}
704+
github := &fakeGitHub{runner: idleRunner, found: true, listRunners: []gh.Runner{idleRunner}}
705+
manager.Provider = fake
706+
manager.Lifecycle = provider.AdaptLegacy(fake, false)
707+
manager.GitHub = github
708+
known := map[string]ProvisionedInstance{name: {Name: name, ProviderID: "docker:ready-id", RunnerID: 42, ProviderOwned: true, Phase: LifecycleQuarantined}}
709+
710+
active, err := manager.reconcilePhysicalPool(context.Background(), known, true)
711+
if err != nil {
712+
t.Fatal(err)
713+
}
714+
if got := active[name].Phase; got != LifecycleReady {
715+
t.Fatalf("recovered phase = %s, want %s", got, LifecycleReady)
716+
}
717+
if got := atomic.LoadInt32(&github.deleteCalls); got != 0 {
718+
t.Fatalf("GitHub deletion calls = %d, want zero for transient quarantine recovery", got)
719+
}
720+
if got := atomic.LoadInt32(&fake.deleteCalls); got != 0 {
721+
t.Fatalf("provider deletion calls = %d, want zero for transient quarantine recovery", got)
722+
}
723+
record, err := store.Read(context.Background(), name)
724+
if err != nil {
725+
t.Fatal(err)
726+
}
727+
if record.Phase != poolstate.PhaseReady {
728+
t.Fatalf("durable phase = %s, want unchanged %s", record.Phase, poolstate.PhaseReady)
729+
}
730+
}
731+
495732
func TestHostTrustReconciliationFencesRegistrationWhenIdleLeaseRefreshFails(t *testing.T) {
496733
fake := &fakeProvider{execErrs: []error{errors.New("lease transport unavailable"), nil}}
497734
github := &fakeGitHub{runner: gh.Runner{Name: "runner-1", ID: 42, Status: "online"}, found: true}

‎internal/pool/manager.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1049,6 +1049,34 @@ func (m *Manager) reconcilePhysicalPool(ctx context.Context, known map[string]Pr
10491049
reconciled[name] = vm
10501050
return reconciled, fmt.Errorf("record GitHub job phase for %s: %w", name, err)
10511051
}
1052+
if vm.Phase == LifecycleQuarantined && m.LifecycleState != nil {
1053+
record, recordErr := m.LifecycleState.Read(ctx, name)
1054+
if recordErr != nil {
1055+
reconciled[name] = vm
1056+
return reconciled, fmt.Errorf("read durable quarantine for %s: %w", name, recordErr)
1057+
}
1058+
if record.Phase == poolstate.PhaseQuarantined {
1059+
if runner.Busy {
1060+
// A busy GitHub runner cannot be deleted. Keep the exact
1061+
// capacity quarantined and let its unrefreshed host-trust
1062+
// lease expire closed instead of re-adopting it and
1063+
// repeating the same fence on every reconciliation.
1064+
reconciled[name] = vm
1065+
continue
1066+
}
1067+
if err := m.retireInstance(ctx, vm, "durably quarantined runner became idle"); err != nil {
1068+
if errors.Is(err, provider.ErrControlPlaneFailure) {
1069+
return reconciled, err
1070+
}
1071+
vm.Phase = LifecycleCleanupPending
1072+
reconciled[name] = vm
1073+
m.warnf("[%s] durably quarantined retirement pending: %v\n", name, err)
1074+
} else {
1075+
delete(reconciled, name)
1076+
}
1077+
continue
1078+
}
1079+
}
10521080
if runner.Status == "online" {
10531081
vm.Phase = LifecycleReady
10541082
reconciled[name] = vm

‎internal/pool/provider_lifecycle.go‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -76,23 +76,21 @@ func (m *Manager) verifyProviderRuntime(ctx context.Context, instance provider.I
7676
return nil
7777
}
7878

79-
// verifyProviderHostTrustRuntime preserves the common runtime check and adds
80-
// an optional provider-specific read-only trust-transport check. Providers
81-
// that only implement HostTrustRuntimeActivator therefore retain the common
82-
// VerifyRuntime fallback, while providers with a transport that needs a
83-
// stronger proof can implement HostTrustRuntimeVerifier.
79+
// verifyProviderHostTrustRuntime prefers the provider-specific read-only
80+
// trust-transport check when one exists. General VerifyRuntime implementations
81+
// may validate pristine pre-job state that a running workflow is allowed to
82+
// change, so they are not composed with the dedicated steady-state verifier.
83+
// Providers that only implement HostTrustRuntimeActivator retain the common
84+
// VerifyRuntime fallback.
8485
func (m *Manager) verifyProviderHostTrustRuntime(ctx context.Context, instance provider.Instance) error {
85-
if err := m.verifyProviderRuntime(ctx, instance); err != nil {
86-
return err
87-
}
8886
verifier, ok := m.providerLifecycle().(provider.HostTrustRuntimeVerifier)
89-
if !ok {
87+
if ok {
88+
if err := verifier.VerifyHostTrustRuntime(ctx, instance); err != nil {
89+
return fmt.Errorf("verify provider host-trust runtime: %w", err)
90+
}
9091
return nil
9192
}
92-
if err := verifier.VerifyHostTrustRuntime(ctx, instance); err != nil {
93-
return fmt.Errorf("verify provider host-trust runtime: %w", err)
94-
}
95-
return nil
93+
return m.verifyProviderRuntime(ctx, instance)
9694
}
9795

9896
func (m *Manager) verifyProviderAdmission(ctx context.Context, instance provider.Instance) error {

0 commit comments

Comments
 (0)