proxy: decouple from sandbox

A proxy is mostly associated with an agent. Decouple it from sandbox
so that we can start it before linking vm with an actual sandbox.

Signed-off-by: Peng Tao <bergwolf@gmail.com>

virtcontainers/cc_proxy.go

@@ -5,33 +5,27 @@
 
 package virtcontainers
 
-import (
-	"fmt"
-	"os/exec"
-)
+import "os/exec"
 
 type ccProxy struct {
 }
 
 // start is the proxy start implementation for ccProxy.
-func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
-	if sandbox.config == nil {
-		return -1, "", fmt.Errorf("Sandbox config cannot be nil")
-	}
-
-	config := sandbox.config.ProxyConfig
-	if err := validateProxyConfig(config); err != nil {
+func (p *ccProxy) start(params proxyParams) (int, string, error) {
+	if err := validateProxyParams(params); err != nil {
 		return -1, "", err
 	}
 
+	params.logger.Info("Starting cc proxy")
+
 	// construct the socket path the proxy instance will use
-	proxyURL, err := defaultProxyURL(sandbox, SocketTypeUNIX)
+	proxyURL, err := defaultProxyURL(params.id, SocketTypeUNIX)
 	if err != nil {
 		return -1, "", err
 	}
 
-	args := []string{config.Path, "-uri", proxyURL}
-	if config.Debug {
+	args := []string{params.path, "-uri", proxyURL}
+	if params.debug {
 		args = append(args, "-log", "debug")
 	}
 
@@ -43,7 +37,7 @@ func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, erro
 	return cmd.Process.Pid, proxyURL, nil
 }
 
-func (p *ccProxy) stop(sandbox *Sandbox, pid int) error {
+func (p *ccProxy) stop(pid int) error {
 	return nil
 }
 

virtcontainers/cc_proxy_test.go

@@ -12,5 +12,5 @@ import (
 func TestCCProxyStart(t *testing.T) {
 	proxy := &ccProxy{}
 
-	testProxyStart(t, nil, proxy, CCProxyType)
+	testProxyStart(t, nil, proxy)
 }

virtcontainers/hyperstart_agent.go

@@ -390,7 +390,11 @@ func (h *hyper) startProxy(sandbox *Sandbox) error {
 	}
 
 	// Start the proxy here
-	pid, uri, err := h.proxy.start(sandbox, proxyParams{})
+	pid, uri, err := h.proxy.start(proxyParams{
+		id:     sandbox.id,
+		path:   sandbox.config.ProxyConfig.Path,
+		logger: h.Logger(),
+	})
 	if err != nil {
 		return err
 	}
@@ -461,7 +465,7 @@ func (h *hyper) stopSandbox(sandbox *Sandbox) error {
 		return err
 	}
 
-	return h.proxy.stop(sandbox, h.state.ProxyPid)
+	return h.proxy.stop(h.state.ProxyPid)
 }
 
 // handleBlockVolumes handles volumes that are block device files, by

virtcontainers/kata_agent.go

@@ -477,13 +477,22 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error {
 		return err
 	}
 
+	consoleURL, err := sandbox.hypervisor.getSandboxConsole(sandbox.id)
+	if err != nil {
+		return err
+	}
+
 	proxyParams := proxyParams{
-		agentURL: agentURL,
-		logger:   k.Logger().WithField("sandbox", sandbox.id),
+		id:         sandbox.id,
+		path:       sandbox.config.ProxyConfig.Path,
+		agentURL:   agentURL,
+		consoleURL: consoleURL,
+		logger:     k.Logger().WithField("sandbox", sandbox.id),
+		debug:      sandbox.config.ProxyConfig.Debug,
 	}
 
 	// Start the proxy here
-	pid, uri, err := k.proxy.start(sandbox, proxyParams)
+	pid, uri, err := k.proxy.start(proxyParams)
 	if err != nil {
 		return err
 	}
@@ -492,7 +501,7 @@ func (k *kataAgent) startProxy(sandbox *Sandbox) error {
 	// then rollback to kill kata-proxy process
 	defer func() {
 		if err != nil && pid > 0 {
-			k.proxy.stop(sandbox, pid)
+			k.proxy.stop(pid)
 		}
 	}()
 
@@ -602,7 +611,7 @@ func (k *kataAgent) stopSandbox(sandbox *Sandbox) error {
 		return err
 	}
 
-	return k.proxy.stop(sandbox, k.state.ProxyPid)
+	return k.proxy.stop(k.state.ProxyPid)
 }
 
 func (k *kataAgent) cleanupSandbox(sandbox *Sandbox) error {

virtcontainers/kata_builtin_proxy.go

@@ -26,22 +26,36 @@ func (p *kataBuiltInProxy) consoleWatched() bool {
 	return p.conn != nil
 }
 
+func (p *kataBuiltInProxy) validateParams(params proxyParams) error {
+	if len(params.id) == 0 || len(params.agentURL) == 0 || len(params.consoleURL) == 0 {
+		return fmt.Errorf("Invalid proxy parameters %+v", params)
+	}
+
+	if params.logger == nil {
+		return fmt.Errorf("Invalid proxy parameter: proxy logger is not set")
+	}
+
+	return nil
+}
+
 // start is the proxy start implementation for kata builtin proxy.
 // It starts the console watcher for the guest.
 // It returns agentURL to let agent connect directly.
-func (p *kataBuiltInProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
-	if p.consoleWatched() {
-		return -1, "", fmt.Errorf("kata builtin proxy running for sandbox %s", p.sandboxID)
-	}
-
-	p.sandboxID = sandbox.id
-	console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id)
-	if err != nil {
+func (p *kataBuiltInProxy) start(params proxyParams) (int, string, error) {
+	if err := p.validateParams(params); err != nil {
 		return -1, "", err
 	}
 
-	err = p.watchConsole(consoleProtoUnix, console, params.logger)
+	if p.consoleWatched() {
+		return -1, "", fmt.Errorf("kata builtin proxy running for sandbox %s", params.id)
+	}
+
+	params.logger.Info("Starting builtin kata proxy")
+
+	p.sandboxID = params.id
+	err := p.watchConsole(consoleProtoUnix, params.consoleURL, params.logger)
 	if err != nil {
+		p.sandboxID = ""
 		return -1, "", err
 	}
 
@@ -49,7 +63,7 @@ func (p *kataBuiltInProxy) start(sandbox *Sandbox, params proxyParams) (int, str
 }
 
 // stop is the proxy stop implementation for kata builtin proxy.
-func (p *kataBuiltInProxy) stop(sandbox *Sandbox, pid int) error {
+func (p *kataBuiltInProxy) stop(pid int) error {
 	if p.conn != nil {
 		p.conn.Close()
 		p.conn = nil

virtcontainers/kata_proxy.go

@@ -6,7 +6,6 @@
 package virtcontainers
 
 import (
-	"fmt"
 	"os/exec"
 	"syscall"
 )
@@ -23,46 +22,28 @@ func (p *kataProxy) consoleWatched() bool {
 }
 
 // start is kataProxy start implementation for proxy interface.
-func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
-	sandbox.Logger().Info("Starting regular Kata proxy rather than built-in")
-	if sandbox.config == nil {
-		return -1, "", fmt.Errorf("Sandbox config cannot be nil")
-	}
-
-	if sandbox.agent == nil {
-		return -1, "", fmt.Errorf("No agent")
-	}
-
-	if params.agentURL == "" {
-		return -1, "", fmt.Errorf("AgentURL cannot be empty")
-	}
-
-	config := sandbox.config.ProxyConfig
-	if err := validateProxyConfig(config); err != nil {
+func (p *kataProxy) start(params proxyParams) (int, string, error) {
+	if err := validateProxyParams(params); err != nil {
 		return -1, "", err
 	}
 
+	params.logger.Info("Starting regular Kata proxy rather than built-in")
+
 	// construct the socket path the proxy instance will use
-	proxyURL, err := defaultProxyURL(sandbox, SocketTypeUNIX)
+	proxyURL, err := defaultProxyURL(params.id, SocketTypeUNIX)
 	if err != nil {
 		return -1, "", err
 	}
 
 	args := []string{
-		config.Path,
+		params.path,
 		"-listen-socket", proxyURL,
 		"-mux-socket", params.agentURL,
-		"-sandbox", sandbox.ID(),
+		"-sandbox", params.id,
 	}
 
-	if config.Debug {
-		args = append(args, "-log", "debug")
-		console, err := sandbox.hypervisor.getSandboxConsole(sandbox.id)
-		if err != nil {
-			return -1, "", err
-		}
-
-		args = append(args, "-agent-logs-socket", console)
+	if params.debug {
+		args = append(args, "-log", "debug", "-agent-logs-socket", params.consoleURL)
 	}
 
 	cmd := exec.Command(args[0], args[1:]...)
@@ -74,7 +55,7 @@ func (p *kataProxy) start(sandbox *Sandbox, params proxyParams) (int, string, er
 }
 
 // stop is kataProxy stop implementation for proxy interface.
-func (p *kataProxy) stop(sandbox *Sandbox, pid int) error {
+func (p *kataProxy) stop(pid int) error {
 	// Signal the proxy with SIGTERM.
 	return syscall.Kill(pid, syscall.SIGTERM)
 }

virtcontainers/kata_proxy_test.go

@@ -13,5 +13,5 @@ func TestKataProxyStart(t *testing.T) {
 	agent := &kataAgent{}
 	proxy := &kataProxy{}
 
-	testProxyStart(t, agent, proxy, KataProxyType)
+	testProxyStart(t, agent, proxy)
 }

virtcontainers/no_proxy.go

@@ -23,8 +23,13 @@ type noProxy struct {
 }
 
 // start is noProxy start implementation for proxy interface.
-func (p *noProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
-	sandbox.Logger().Info("No proxy started because of no-proxy implementation")
+func (p *noProxy) start(params proxyParams) (int, string, error) {
+	if params.logger == nil {
+		return -1, "", fmt.Errorf("proxy logger is not set")
+	}
+
+	params.logger.Info("No proxy started because of no-proxy implementation")
+
 	if params.agentURL == "" {
 		return -1, "", fmt.Errorf("AgentURL cannot be empty")
 	}
@@ -33,7 +38,7 @@ func (p *noProxy) start(sandbox *Sandbox, params proxyParams) (int, string, erro
 }
 
 // stop is noProxy stop implementation for proxy interface.
-func (p *noProxy) stop(sandbox *Sandbox, pid int) error {
+func (p *noProxy) stop(pid int) error {
 	return nil
 }
 

virtcontainers/no_proxy_test.go

@@ -10,14 +10,13 @@ import (
 )
 
 func TestNoProxyStart(t *testing.T) {
-	sandbox := &Sandbox{
-		agent: newAgent(NoopAgentType),
-	}
-
 	p := &noProxy{}
 
 	agentURL := "agentURL"
-	pid, vmURL, err := p.start(sandbox, proxyParams{agentURL: agentURL})
+	pid, vmURL, err := p.start(proxyParams{
+		agentURL: agentURL,
+		logger:   testDefaultLogger,
+	})
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -34,7 +33,7 @@ func TestNoProxyStart(t *testing.T) {
 func TestNoProxyStop(t *testing.T) {
 	p := &noProxy{}
 
-	if err := p.stop(&Sandbox{}, 0); err != nil {
+	if err := p.stop(0); err != nil {
 		t.Fatal(err)
 	}
 }

virtcontainers/noop_proxy.go

@@ -13,13 +13,13 @@ var noopProxyURL = "noopProxyURL"
 
 // register is the proxy start implementation for testing purpose.
 // It does nothing.
-func (p *noopProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
+func (p *noopProxy) start(params proxyParams) (int, string, error) {
 	return 0, noopProxyURL, nil
 }
 
 // stop is the proxy stop implementation for testing purpose.
 // It does nothing.
-func (p *noopProxy) stop(sandbox *Sandbox, pid int) error {
+func (p *noopProxy) stop(pid int) error {
 	return nil
 }
 

virtcontainers/proxy.go

@@ -22,8 +22,12 @@ type ProxyConfig struct {
 // proxyParams is the structure providing specific parameters needed
 // for the execution of the proxy binary.
 type proxyParams struct {
-	agentURL string
-	logger   *logrus.Entry
+	id         string
+	path       string
+	agentURL   string
+	consoleURL string
+	logger     *logrus.Entry
+	debug      bool
 }
 
 // ProxyType describes a proxy type.
@@ -119,6 +123,18 @@ func newProxy(pType ProxyType) (proxy, error) {
 	}
 }
 
+func validateProxyParams(p proxyParams) error {
+	if len(p.path) == 0 || len(p.id) == 0 || len(p.agentURL) == 0 || len(p.consoleURL) == 0 {
+		return fmt.Errorf("Invalid proxy parameters %+v", p)
+	}
+
+	if p.logger == nil {
+		return fmt.Errorf("Invalid proxy parameter: proxy logger is not set")
+	}
+
+	return nil
+}
+
 func validateProxyConfig(proxyConfig ProxyConfig) error {
 	if len(proxyConfig.Path) == 0 {
 		return fmt.Errorf("Proxy path cannot be empty")
@@ -127,10 +143,10 @@ func validateProxyConfig(proxyConfig ProxyConfig) error {
 	return nil
 }
 
-func defaultProxyURL(sandbox *Sandbox, socketType string) (string, error) {
+func defaultProxyURL(id, socketType string) (string, error) {
 	switch socketType {
 	case SocketTypeUNIX:
-		socketPath := filepath.Join(runStoragePath, sandbox.id, "proxy.sock")
+		socketPath := filepath.Join(runStoragePath, id, "proxy.sock")
 		return fmt.Sprintf("unix://%s", socketPath), nil
 	case SocketTypeVSOCK:
 		// TODO Build the VSOCK default URL
@@ -146,13 +162,13 @@ func isProxyBuiltIn(pType ProxyType) bool {
 
 // proxy is the virtcontainers proxy interface.
 type proxy interface {
-	// start launches a proxy instance for the specified sandbox, returning
+	// start launches a proxy instance with specified parameters, returning
 	// the PID of the process and the URL used to connect to it.
-	start(sandbox *Sandbox, params proxyParams) (int, string, error)
+	start(params proxyParams) (int, string, error)
 
 	// stop terminates a proxy instance after all communications with the
 	// agent inside the VM have been properly stopped.
-	stop(sandbox *Sandbox, pid int) error
+	stop(pid int) error
 
 	//check if the proxy has watched the vm console.
 	consoleWatched() bool

virtcontainers/proxy_test.go

@@ -13,9 +13,12 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 )
 
+var testDefaultLogger = logrus.WithField("proxy", "test")
+
 func testSetProxyType(t *testing.T, value string, expected ProxyType) {
 	var proxyType ProxyType
 
@@ -206,7 +209,7 @@ func testDefaultProxyURL(expectedURL string, socketType string, sandboxID string
 		id: sandboxID,
 	}
 
-	url, err := defaultProxyURL(sandbox, socketType)
+	url, err := defaultProxyURL(sandbox.id, socketType)
 	if err != nil {
 		return err
 	}
@@ -242,7 +245,7 @@ func TestDefaultProxyURLUnknown(t *testing.T) {
 	}
 }
 
-func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType) {
+func testProxyStart(t *testing.T, agent agent, proxy proxy) {
 	assert := assert.New(t)
 
 	assert.NotNil(proxy)
@@ -252,7 +255,6 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType)
 	defer os.RemoveAll(tmpdir)
 
 	type testData struct {
-		sandbox     *Sandbox
 		params      proxyParams
 		expectedURI string
 		expectError bool
@@ -263,60 +265,43 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType)
 	expectedURI := fmt.Sprintf("unix://%s", expectedSocketPath)
 
 	data := []testData{
-		{&Sandbox{}, proxyParams{}, "", true},
+		{proxyParams{}, "", true},
 		{
-			&Sandbox{
-				config: &SandboxConfig{
-					ProxyType: "invalid",
-				},
-			},
-			proxyParams{},
-			"", true,
-		},
-		{
-			&Sandbox{
-				config: &SandboxConfig{
-					ProxyType: proxyType,
-					// invalid - no path
-					ProxyConfig: ProxyConfig{},
-				},
-			},
-			proxyParams{},
-			"", true,
-		},
-		{
-			&Sandbox{
-				config: &SandboxConfig{
-					ProxyType: proxyType,
-					ProxyConfig: ProxyConfig{
-						Path: invalidPath,
-					},
-				},
-			},
-			proxyParams{},
-			"", true,
-		},
-
-		{
-			&Sandbox{
-				id:    testSandboxID,
-				agent: agent,
-				config: &SandboxConfig{
-					ProxyType: proxyType,
-					ProxyConfig: ProxyConfig{
-						Path: "echo",
-					},
-				},
-			},
+			// no path
 			proxyParams{
-				agentURL: "agentURL",
+				id:         "foobar",
+				agentURL:   "agentURL",
+				consoleURL: "consoleURL",
+				logger:     testDefaultLogger,
+			},
+			"", true,
+		},
+		{
+			// invalid path
+			proxyParams{
+				id:         "foobar",
+				path:       invalidPath,
+				agentURL:   "agentURL",
+				consoleURL: "consoleURL",
+				logger:     testDefaultLogger,
+			},
+			"", true,
+		},
+		{
+			// good case
+			proxyParams{
+				id:         testSandboxID,
+				path:       "echo",
+				agentURL:   "agentURL",
+				consoleURL: "consoleURL",
+				logger:     testDefaultLogger,
 			},
 			expectedURI, false,
 		},
 	}
 
 	for _, d := range data {
-		pid, uri, err := proxy.start(d.sandbox, d.params)
+		pid, uri, err := proxy.start(d.params)
 		if d.expectError {
 			assert.Error(err)
 			continue
@@ -327,3 +312,43 @@ func testProxyStart(t *testing.T, agent agent, proxy proxy, proxyType ProxyType)
 		assert.Equal(d.expectedURI, uri)
 	}
 }
+
+func TestValidateProxyConfig(t *testing.T) {
+	assert := assert.New(t)
+
+	config := ProxyConfig{}
+	err := validateProxyConfig(config)
+	assert.Error(err)
+
+	config.Path = "foobar"
+	err = validateProxyConfig(config)
+	assert.Nil(err)
+}
+
+func TestValidateProxyParams(t *testing.T) {
+	assert := assert.New(t)
+
+	p := proxyParams{}
+	err := validateProxyParams(p)
+	assert.Error(err)
+
+	p.path = "foobar"
+	err = validateProxyParams(p)
+	assert.Error(err)
+
+	p.id = "foobar1"
+	err = validateProxyParams(p)
+	assert.Error(err)
+
+	p.agentURL = "foobar2"
+	err = validateProxyParams(p)
+	assert.Error(err)
+
+	p.consoleURL = "foobar3"
+	err = validateProxyParams(p)
+	assert.Error(err)
+
+	p.logger = &logrus.Entry{}
+	err = validateProxyParams(p)
+	assert.Nil(err)
+}