.golangci.yml

@@ -9,6 +9,7 @@ formatters:
     - gofmt
     - gofumpt
     - goimports
+    - golines
     - swaggo
 
 linters:
@@ -40,7 +41,6 @@ linters:
 
     # Style linters that are total nuts.
     - wsl
-    - wsl_v5
     - funlen
 
     # Enough parallelism for now.
@@ -70,18 +70,6 @@ linters:
     # Not all errors need to be checked
     - errcheck
 
-    # It's OK to not initialize some struct fields
-    - exhaustruct
-
-    # We'll control the variable name length ourselves
-    - varnamelen
-
-    # Inline error handling keeps assignment and checking together
-    - noinlineerr
-
-    # Avoid unrelated style churn for now
-    - funcorder
-
 issues:
   # Don't hide multiple issues that belong to one class since GitHub annotations can handle them all nicely.
   max-issues-per-linter: 0

api/openapi.yaml

@@ -791,9 +791,7 @@ components:
             model: macstudio
         hostDirs:
           type: array
-          description: |
-            Directories on the Orchard Worker host to mount to a VM.
-            Requires running Orchard Controller with `--insecure-allow-host-dirs`.
+          description: Directories on the Orchard Worker host to mount to a VM
           items:
             type: object
             properties:

internal/command/controller/run.go

@@ -30,7 +30,6 @@ var addressPprof string
 var debug bool
 var noTLS bool
 var sshNoClientAuth bool
-var insecureAllowHostDirs bool
 var experimentalRPCV2 bool
 var noExperimentalRPCV2 bool
 var experimentalPingInterval time.Duration
@@ -75,8 +74,6 @@ func newRunCommand() *cobra.Command {
 	cmd.Flags().BoolVar(&sshNoClientAuth, "insecure-ssh-no-client-auth", false,
 		"allow SSH clients to connect to the controller's SSH server without authentication, "+
 			"thus only authenticating on the target worker/VM's SSH server")
-	cmd.Flags().BoolVar(&insecureAllowHostDirs, "insecure-allow-host-dirs", false,
-		"allow unsafe path-based local host directory sharing")
 	cmd.Flags().BoolVar(&experimentalRPCV2, "experimental-rpc-v2", false,
 		"enable experimental RPC v2 (https://github.com/cirruslabs/orchard/issues/235)")
 	_ = cmd.Flags().MarkHidden("experimental-rpc-v2")
@@ -169,10 +166,6 @@ func runController(cmd *cobra.Command, args []string) (err error) {
 		controllerOpts = append(controllerOpts, controller.WithSynthetic())
 	}
 
-	if insecureAllowHostDirs {
-		controllerOpts = append(controllerOpts, controller.WithInsecureAllowHostDirs())
-	}
-
 	var controllerCert tls.Certificate
 
 	if !noTLS {

internal/command/dev/dev.go

@@ -35,7 +35,6 @@ var experimentalRPCV2 bool
 var addressPprof string
 var synthetic bool
 var workers int
-var insecureAllowHostDirs bool
 
 func NewCommand() *cobra.Command {
 	command := &cobra.Command{
@@ -58,8 +57,6 @@ func NewCommand() *cobra.Command {
 	command.Flags().BoolVar(&synthetic, "synthetic", false,
 		"do not instantiate real Tart VM, use synthetic in-memory VMs suitable for load testing")
 	command.Flags().IntVar(&workers, "workers", 1, "number of workers to start")
-	command.Flags().BoolVar(&insecureAllowHostDirs, "insecure-allow-host-dirs", false,
-		"allow unsafe path-based local host directory sharing")
 
 	return command
 }
@@ -108,10 +105,6 @@ func runDev(cmd *cobra.Command, args []string) error {
 		additionalControllerOpts = append(additionalControllerOpts, controller.WithExperimentalRPCV2())
 	}
 
-	if insecureAllowHostDirs {
-		additionalControllerOpts = append(additionalControllerOpts, controller.WithInsecureAllowHostDirs())
-	}
-
 	group, ctx := errgroup.WithContext(cmd.Context())
 
 	var additionalWorkerOpts []worker.Option

internal/controller/api_vms.go

@@ -158,10 +158,6 @@ func (controller *Controller) updateVMSpec(ctx *gin.Context) responder.Responder
 		return responder.JSON(http.StatusBadRequest, NewErrorResponse("invalid JSON was provided"))
 	}
 
-	if responder := controller.validateHostDirs(userVM.HostDirs); responder != nil {
-		return responder
-	}
-
 	name := ctx.Param("name")
 
 	return controller.storeUpdate(func(txn storepkg.Transaction) responder.Responder {
@@ -544,14 +540,6 @@ func (controller *Controller) validateHostDirs(hostDirs []v1.HostDir) responder.
 		return nil
 	}
 
-	if !controller.insecureAllowHostDirs {
-		return responder.JSON(
-			http.StatusBadRequest,
-			NewErrorResponse("host directory sharing is disabled; "+
-				"restart the controller with --insecure-allow-host-dirs to enable this unsafe feature"),
-		)
-	}
-
 	// Retrieve cluster settings
 	var clusterSettings *v1.ClusterSettings
 	var err error

internal/controller/controller.go

@@ -46,7 +46,6 @@ type Controller struct {
 	listener                           net.Listener
 	httpServer                         *http.Server
 	insecureAuthDisabled               bool
-	insecureAllowHostDirs              bool
 	scheduler                          *scheduler.Scheduler
 	store                              storepkg.Store
 	logger                             *zap.SugaredLogger
@@ -196,14 +195,6 @@ func New(opts ...Option) (*Controller, error) {
 		return nil, err
 	}
 
-	// When no "--insecure-allow-host-dirs" is present,
-	// fail the VMs that have "hostDirs" set
-	if !controller.insecureAllowHostDirs {
-		if err := controller.failVMsWithHostDirs(); err != nil {
-			return nil, err
-		}
-	}
-
 	return controller, nil
 }
 
@@ -314,34 +305,6 @@ func (controller *Controller) DeleteServiceAccount(name string) error {
 	})
 }
 
-func (controller *Controller) failVMsWithHostDirs() error {
-	return controller.store.Update(func(txn storepkg.Transaction) error {
-		vms, err := txn.ListVMs()
-		if err != nil {
-			return err
-		}
-
-		for _, vm := range vms {
-			permanentlyFailed := vm.TerminalState() &&
-				vm.RestartPolicy == v1.RestartPolicyNever
-
-			if permanentlyFailed || len(vm.HostDirs) == 0 {
-				continue
-			}
-
-			vm.Status = v1.VMStatusFailed
-			vm.StatusMessage = "host directories are used, but host directory sharing is disabled"
-			vm.RestartPolicy = v1.RestartPolicyNever
-
-			if err := txn.SetVM(vm); err != nil {
-				return err
-			}
-		}
-
-		return nil
-	})
-}
-
 func (controller *Controller) Run(ctx context.Context) error {
 	// Run the scheduler so that each VM will eventually
 	// be assigned to a specific Worker

internal/controller/option.go

@@ -48,12 +48,6 @@ func WithInsecureAuthDisabled() Option {
 	}
 }
 
-func WithInsecureAllowHostDirs() Option {
-	return func(controller *Controller) {
-		controller.insecureAllowHostDirs = true
-	}
-}
-
 func WithSwaggerDocs() Option {
 	return func(controller *Controller) {
 		controller.enableSwaggerDocs = true

internal/tests/integration_test.go

@@ -372,26 +372,12 @@ func TestVMGarbageCollection(t *testing.T) {
 	}), "failed to wait for the VM %s to be garbage-collected", vmName)
 }
 
-func TestHostDirsDisabledByDefault(t *testing.T) {
-	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
-
-	err := devClient.VMs().Create(context.Background(), &v1.VM{
-		Meta:     v1.Meta{Name: "test-host-dirs-disabled"},
-		Image:    imageconstant.DefaultMacosImage,
-		HostDirs: []v1.HostDir{{Name: "src", Path: "/Users/ci/src"}},
-	})
-	require.Error(t, err)
-}
-
 func TestHostDirs(t *testing.T) {
 	if runtime.GOOS != "darwin" {
 		t.Skip("HostDirs is only supported on macOS with Tart")
 	}
 
-	devClient, _, _ := devcontroller.StartIntegrationTestEnvironmentWithAdditionalOpts(t,
-		false, []controller.Option{controller.WithInsecureAllowHostDirs()},
-		false, nil,
-	)
+	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
 
 	dirToMount := t.TempDir()
 
@@ -463,10 +449,7 @@ func TestHostDirsInvalidPolicy(t *testing.T) {
 		t.Skip("HostDirs is only supported on macOS with Tart")
 	}
 
-	devClient, _, _ := devcontroller.StartIntegrationTestEnvironmentWithAdditionalOpts(t,
-		false, []controller.Option{controller.WithInsecureAllowHostDirs()},
-		false, nil,
-	)
+	devClient, _, _ := devcontroller.StartIntegrationTestEnvironment(t)
 
 	dirToMount := t.TempDir()
 

internal/worker/worker.go

@@ -475,16 +475,12 @@ func (worker *Worker) syncVMs(ctx context.Context, updateVM func(context.Context
 		case ActionStop:
 			// VM has failed on the remote side, stop it locally to prevent incorrect
 			// worker's resources calculation in the Controller's scheduler
-			if err := waitForVMStop(ctx, vm); err != nil {
-				return fmt.Errorf("failed to stop VM: %w", err)
-			}
+			vm.Stop()
 		case ActionFail, ActionLostTrack, ActionImpossible:
 			// VM has failed on the local side, stop it before reporting as failed to prevent incorrect
 			// worker's resources calculation in the Controller's scheduler
 			if vm != nil {
-				if err := waitForVMStop(ctx, vm); err != nil {
-					return fmt.Errorf("failed to stop VM: %w", err)
-				}
+				vm.Stop()
 			}
 
 			var statusMessage string
@@ -586,15 +582,6 @@ func (worker *Worker) syncOnDiskVMs(ctx context.Context) error {
 	return nil
 }
 
-func waitForVMStop(ctx context.Context, vm vmmanager.VM) error {
-	select {
-	case err := <-vm.Stop():
-		return err
-	case <-ctx.Done():
-		return ctx.Err()
-	}
-}
-
 func (worker *Worker) deleteVM(vm vmmanager.VM) error {
 	<-vm.Stop()
 

internal/worker/worker_stop_test.go

@@ -1,135 +0,0 @@
-package worker //nolint:testpackage // The regression test exercises unexported worker reconciliation.
-
-import (
-	"context"
-	"encoding/json"
-	"errors"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-	"time"
-
-	"github.com/cirruslabs/orchard/internal/worker/ondiskname"
-	"github.com/cirruslabs/orchard/internal/worker/vmmanager"
-	"github.com/cirruslabs/orchard/pkg/client"
-	v1 "github.com/cirruslabs/orchard/pkg/resource/v1"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/zap"
-)
-
-var errLocalVMFailed = errors.New("local VM failed")
-
-type delayedStopVM struct {
-	vmmanager.VM
-
-	resource    v1.VM
-	status      v1.VMStatus
-	stopStarted chan struct{}
-	stopResult  chan error
-}
-
-func (vm *delayedStopVM) OnDiskName() ondiskname.OnDiskName {
-	return ondiskname.NewFromResource(vm.resource)
-}
-
-func (vm *delayedStopVM) Status() v1.VMStatus { return vm.status }
-
-func (vm *delayedStopVM) Conditions() []v1.Condition { return nil }
-
-func (vm *delayedStopVM) Err() error { return errLocalVMFailed }
-
-func (vm *delayedStopVM) Stop() <-chan error {
-	close(vm.stopStarted)
-	return vm.stopResult
-}
-
-func TestSyncVMsWaitsForVMShutdown(t *testing.T) {
-	tests := []struct {
-		name         string
-		remoteStatus v1.VMStatus
-		localStatus  v1.VMStatus
-		update       bool
-	}{
-		{
-			name:         "remote failed VM stops before reconciliation continues",
-			remoteStatus: v1.VMStatusFailed,
-			localStatus:  v1.VMStatusRunning,
-		},
-		{
-			name:         "local failed VM stops before reporting failure",
-			remoteStatus: v1.VMStatusRunning,
-			localStatus:  v1.VMStatusFailed,
-			update:       true,
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			resource := v1.VM{
-				Meta:   v1.Meta{Name: "test-vm"},
-				UID:    "00112233-4455-6677-8899-aabbccddeeff",
-				Worker: "test-worker",
-				Status: test.remoteStatus,
-			}
-
-			server := httptest.NewServer(http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
-				if request.Method != http.MethodGet || request.URL.Path != "/v1/vms" {
-					t.Errorf("unexpected request: %s %s", request.Method, request.URL.Path)
-					return
-				}
-
-				response.Header().Set("Content-Type", "application/json")
-				if err := json.NewEncoder(response).Encode([]map[string]any{{
-					"name":   resource.Name,
-					"uid":    resource.UID,
-					"worker": resource.Worker,
-					"status": resource.Status,
-				}}); err != nil {
-					t.Errorf("encode VM response: %v", err)
-				}
-			}))
-			defer server.Close()
-
-			apiClient, err := client.New(client.WithAddress(server.URL))
-			require.NoError(t, err)
-			vm := &delayedStopVM{
-				resource:    resource,
-				status:      test.localStatus,
-				stopStarted: make(chan struct{}),
-				stopResult:  make(chan error, 1),
-			}
-			manager := vmmanager.New()
-			manager.Put(vm.OnDiskName(), vm)
-			worker := &Worker{name: "test-worker", client: apiClient, vmm: manager, logger: zap.NewNop().Sugar()}
-			updated := make(chan v1.VM, 1)
-			finished := make(chan error, 1)
-
-			go func() {
-				finished <- worker.syncVMs(context.Background(), func(_ context.Context, updatedVM v1.VM) error {
-					updated <- updatedVM
-					return nil
-				})
-			}()
-
-			select {
-			case <-vm.stopStarted:
-			case <-time.After(time.Second):
-				t.Fatal("VM shutdown was not requested")
-			}
-
-			select {
-			case updatedVM := <-updated:
-				t.Fatalf("VM was reported %q before shutdown completed", updatedVM.Status)
-			case err := <-finished:
-				t.Fatalf("reconciliation continued before VM shutdown completed: %v", err)
-			case <-time.After(30 * time.Millisecond):
-			}
-
-			vm.stopResult <- nil
-			require.NoError(t, <-finished)
-			if test.update {
-				require.Equal(t, v1.VMStatusFailed, (<-updated).Status)
-			}
-		})
-	}
-}

pkg/resource/v1/host_dir_policy.go

@@ -58,10 +58,10 @@ func (policy HostDirPolicy) Validate(path string, readOnly bool) bool {
 		return false
 	}
 
-	path = strings.TrimSuffix(path, "/")
-	pathPrefix := strings.TrimSuffix(policy.PathPrefix, "/")
-
-	return path == pathPrefix || strings.HasPrefix(path, pathPrefix+"/")
+	return strings.HasPrefix(
+		strings.TrimSuffix(path, "/"),
+		strings.TrimSuffix(policy.PathPrefix, "/"),
+	)
 }
 
 func (policy HostDirPolicy) String() string {

pkg/resource/v1/host_dir_policy_test.go

@@ -44,107 +44,6 @@ func TestHostDirPolicyValidate(t *testing.T) {
 	require.False(t, policy.Validate("/..", true))
 }
 
-func TestHostDirPolicyValidatePathBoundary(t *testing.T) {
-	const (
-		localPathPrefix = "/src/"
-		githubURLPrefix = "https://github.com"
-	)
-
-	testCases := []struct {
-		name       string
-		pathPrefix string
-		path       string
-		allowed    bool
-	}{
-		{
-			name:       "local policy allows its exact path",
-			pathPrefix: localPathPrefix,
-			path:       "/src",
-			allowed:    true,
-		},
-		{
-			name:       "local policy allows descendants",
-			pathPrefix: localPathPrefix,
-			path:       "/src/project",
-			allowed:    true,
-		},
-		{
-			name:       "local policy without trailing slash rejects sibling sharing its prefix",
-			pathPrefix: "/src",
-			path:       "/src-private",
-			allowed:    false,
-		},
-		{
-			name:       "local policy rejects sibling sharing its prefix",
-			pathPrefix: localPathPrefix,
-			path:       "/src-private",
-			allowed:    false,
-		},
-		{
-			name:       "local root policy allows descendants",
-			pathPrefix: "/",
-			path:       "/src/project",
-			allowed:    true,
-		},
-		{
-			name:       "local root policy rejects remote URLs",
-			pathPrefix: "/",
-			path:       "https://github.com/archive.tar.gz",
-			allowed:    false,
-		},
-		{
-			name:       "URL policy allows its exact host",
-			pathPrefix: githubURLPrefix + "/",
-			path:       githubURLPrefix,
-			allowed:    true,
-		},
-		{
-			name:       "URL policy allows paths on its host",
-			pathPrefix: githubURLPrefix,
-			path:       "https://github.com/actions/archive.tar.gz",
-			allowed:    true,
-		},
-		{
-			name:       "URL policy rejects lookalike host",
-			pathPrefix: githubURLPrefix,
-			path:       "https://github.com.attacker.com/archive.tar.gz",
-			allowed:    false,
-		},
-		{
-			name:       "URL policy with trailing slash rejects lookalike host",
-			pathPrefix: githubURLPrefix + "/",
-			path:       "https://github.com.attacker.com/archive.tar.gz",
-			allowed:    false,
-		},
-		{
-			name:       "URL policy rejects host concealed by userinfo",
-			pathPrefix: githubURLPrefix,
-			path:       "https://github.com@attacker.example/archive.tar.gz",
-			allowed:    false,
-		},
-		{
-			name:       "URL path policy allows descendants",
-			pathPrefix: "https://github.com/actions",
-			path:       "https://github.com/actions/runner/archive.tar.gz",
-			allowed:    true,
-		},
-		{
-			name:       "URL path policy rejects sibling sharing its prefix",
-			pathPrefix: "https://github.com/actions",
-			path:       "https://github.com/actions-private/archive.tar.gz",
-			allowed:    false,
-		},
-	}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			policy := v1.HostDirPolicy{PathPrefix: testCase.pathPrefix, ReadOnly: false}
-
-			require.Equal(t, testCase.allowed, policy.Validate(testCase.path, false))
-		})
-	}
-}
-
 func TestHostDirPolicyValidateReadOnly(t *testing.T) {
 	policy := &v1.HostDirPolicy{PathPrefix: "/Users/ci/src", ReadOnly: true}