diff --git a/virtcontainers/cc_proxy.go b/virtcontainers/cc_proxy.go index 3d66e1fd5..fbfa733f2 100644 --- a/virtcontainers/cc_proxy.go +++ b/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 } diff --git a/virtcontainers/cc_proxy_test.go b/virtcontainers/cc_proxy_test.go index ed1985527..d5b07f85c 100644 --- a/virtcontainers/cc_proxy_test.go +++ b/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) } diff --git a/virtcontainers/hyperstart_agent.go b/virtcontainers/hyperstart_agent.go index 4c135b8bc..c7ee02a3b 100644 --- a/virtcontainers/hyperstart_agent.go +++ b/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 diff --git a/virtcontainers/kata_agent.go b/virtcontainers/kata_agent.go index 41e0d1541..088ee4b15 100644 --- a/virtcontainers/kata_agent.go +++ b/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 { diff --git a/virtcontainers/kata_builtin_proxy.go b/virtcontainers/kata_builtin_proxy.go index ebd3cef0c..58b7c6edd 100644 --- a/virtcontainers/kata_builtin_proxy.go +++ b/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 diff --git a/virtcontainers/kata_proxy.go b/virtcontainers/kata_proxy.go index e68686295..1ff9d01f1 100644 --- a/virtcontainers/kata_proxy.go +++ b/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) } diff --git a/virtcontainers/kata_proxy_test.go b/virtcontainers/kata_proxy_test.go index 0e202d7a5..d14fc7bcb 100644 --- a/virtcontainers/kata_proxy_test.go +++ b/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) } diff --git a/virtcontainers/no_proxy.go b/virtcontainers/no_proxy.go index 33664a56b..ef97e4a30 100644 --- a/virtcontainers/no_proxy.go +++ b/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 } diff --git a/virtcontainers/no_proxy_test.go b/virtcontainers/no_proxy_test.go index 4b1bd1b63..5bf6df765 100644 --- a/virtcontainers/no_proxy_test.go +++ b/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) } } diff --git a/virtcontainers/noop_proxy.go b/virtcontainers/noop_proxy.go index 473d8a63a..5a80f7b60 100644 --- a/virtcontainers/noop_proxy.go +++ b/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 } diff --git a/virtcontainers/proxy.go b/virtcontainers/proxy.go index 29256e734..050980c3e 100644 --- a/virtcontainers/proxy.go +++ b/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 diff --git a/virtcontainers/proxy_test.go b/virtcontainers/proxy_test.go index 4f72a2212..4a396e9e9 100644 --- a/virtcontainers/proxy_test.go +++ b/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) +}